Re: dm raid: ensure metadata IO matches device block size.

[NeilBrown <neilb@suse.de>]

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

On Tue, 14 Oct 2014 22:55:50 -0400 Mike Snitzer <snitzer@redhat.com> wrote:

> On Tue, Oct 14 2014 at  9:19pm -0400,
> NeilBrown <neilb@suse.de> wrote:
> 
> > 
> > dm_raid_superblock is 512.
> > Reading or writing this on a 512-byte sector works fine.
> > On a 4096-byte sector device, this fails.
> > 
> > If we round up rdev->sb_size to match the block size of
> > the device, all IO will work correctly.
> > 
> > Reported-by: "Liuhua Wang" <lwang@suse.com>
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > ---
> > this issue has been discussed already a bit. See email thread
> >  Subject: Re: [dm-devel] [PATCH] fix mirror device creation with lvcreate failed
> > I think this is the best fix.  It handles boths read and writes, and (I think)
> > at the best level.
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> > index 4880b69e2e9e..31bdd73bc368 100644
> > --- a/drivers/md/dm-raid.c
> > +++ b/drivers/md/dm-raid.c
> > @@ -858,7 +858,8 @@ static int super_load(struct md_rdev *rdev, struct md_rdev *refdev)
> >  	uint64_t events_sb, events_refsb;
> >  
> >  	rdev->sb_start = 0;
> > -	rdev->sb_size = sizeof(*sb);
> > +	rdev->sb_size = roundup(sizeof(*sb),
> > +				bdev_logical_block_size(rdev->meta_bdev));
> >  
> >  	ret = read_disk_sb(rdev, rdev->sb_size);
> >  	if (ret)
> 
> Wouldn't it be better to use bdev_physical_block_size()?
> 
> Even on a 4K device that emulates 512b logical sectors it is better to
> use the physical block size (4K).


_logical_ is the smallest value for which the IO actually works.
And the goal of the change is to make it work.

I don't object to using _physical_, but it isn't clear to me how I would
justify that as "correct".

A big question in my mind is: how much space does LVM reserve in this device
for the metadata?  It seems reasonable to assume that it reserves at least
1 logical block.  If the API guarantees that at least one physical block is
reserved, then that would justify using _physical_.

A quick look at the code shows that the bitmap superblock is placed 4K after
the start of the metadata.
So the code should probably fail if the rounded-up sb_size exceeds 4K.
Mind you, that would exceed PAGE_SIZE too which would cause other problems.

Maybe use _physical_ unless that exceeds 4K, then try _logical_, then fail if
even that > 4K ??

Thanks,
NeilBrown

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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