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

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 3223 bytes --]

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@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@redhat.com>
> Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@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.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Mike Snitzer]

On Mon, Apr 11 2016 at  9:27pm -0400,
Ben Hutchings <ben@decadent.org.uk> wrote:

> 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@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@redhat.com>
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@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?

Thanks for the report!

I've staged this to go to Linus by the end of the week:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.6&id=a64204672859248c89c8df796421442fb41e59ec