On Mon, Apr 11 2016 at 9:27pm -0400,
Ben Hutchings <ben@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > 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
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
