[dm-devel] [PATCH 4.5 052/238] dm cache: make sure every metadata function checks fail_io

[Ben Hutchings]

On Sun, 2016-04-10 at 11:33 -0700, Greg Kroah-Hartman wrote:
> 4.5-stable review patch.  If anyone has any objections, please let me know.

I've dropped stable because this isn't actually broken, but...

> ------------------
> 
> From: Joe Thornber <ejt at redhat.com>
> 
> commit d14fcf3dd79c0b8a8d0ba469c44a6b04f3a1403b upstream.
> 
> Otherwise operations may be attempted that will only ever go on to crash
> (since the metadata device is either missing or unreliable if 'fail_io'
> is set).
> 
> Signed-off-by: Joe Thornber <ejt at redhat.com>
> Signed-off-by: Mike Snitzer <snitzer at redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> 
> ---
>  drivers/md/dm-cache-metadata.c |   98 ++++++++++++++++++++++++-----------------
>  drivers/md/dm-cache-metadata.h |    4 -
>  drivers/md/dm-cache-target.c   |   12 ++++-
>  3 files changed, 71 insertions(+), 43 deletions(-)
> 
> --- a/drivers/md/dm-cache-metadata.c
> +++ b/drivers/md/dm-cache-metadata.c
> @@ -867,19 +867,40 @@ static int blocks_are_unmapped_or_clean(
>  	return 0;
>  }
>  
> -#define WRITE_LOCK(cmd) \
> -	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) \
> +#define WRITE_LOCK(cmd)	\
> +	down_write(&cmd->root_lock); \
> +	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) { \
> +		up_write(&cmd->root_lock); \
>  		return -EINVAL; \
> -	down_write(&cmd->root_lock)
> +	}
>  
>  #define WRITE_LOCK_VOID(cmd) \
> -	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) \
> +	down_write(&cmd->root_lock); \
> +	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) { \
> +		up_write(&cmd->root_lock); \
>  		return; \
> -	down_write(&cmd->root_lock)
> +	}

Whenever a macro expands to multiple statements they should be wrapped
up in do { ... } while (0) so the macro is safe to use in other
compound statements.

That's not a regression for these existing macros, but:

>  #define WRITE_UNLOCK(cmd) \
>  	up_write(&cmd->root_lock)
>  
> +#define READ_LOCK(cmd) \
> +	down_read(&cmd->root_lock); \
> +	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) { \
> +		up_read(&cmd->root_lock); \
> +		return -EINVAL; \
> +	}
> +
> +#define READ_LOCK_VOID(cmd)	\
> +	down_read(&cmd->root_lock); \
> +	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) { \
> +		up_read(&cmd->root_lock); \
> +		return; \
> +	}
[...]

here you're repeating the same broken pattern in new macros.  Which
checkpatch.pl would have complained about, if you'd thought to run it.

Hiding return statements in macros is another bad idea (who expects
exceptions in C?).  And once we reject that bad idea, all these macros
can be inline functions, like:

static inline bool dm_cm_read_lock(struct dm_cache_metadata *cmd)
{
	down_read(&cmd->root_lock);
	if (cmd->fail_io || dm_bm_is_read_only(cmd->bm)) {
		up_read(&cmd->root_lock);
		return false;
	}
	return true;
}

/* ... */

	if (!dm_cm_read_lock(cmd))
		return -EINVAL;

Actually... I said this wasn't broken, but should the READ_LOCK macros
really fail in case dm_bm_is_read_only(), or only if cmd->fail_io?

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20160412/407549bb/attachment.sig>