On Mon, Apr 11 2016 at 9:27pm -0400, Ben Hutchings <[email protected]> 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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Mike Snitzer <[email protected]> > > Signed-off-by: Greg Kroah-Hartman <[email protected]> > > > > --- > > 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 -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
