On Thu, Oct 16 2014 at 9:31am -0400, Heinz Mauelshagen <heinzm@xxxxxxxxxx> wrote: > > On 10/15/2014 11:00 PM, NeilBrown wrote: > >On Wed, 15 Oct 2014 09:13:08 -0400 Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > > > >>On Tue, Oct 14 2014 at 11:40pm -0400, > >>NeilBrown <neilb@xxxxxxx> wrote: > >> > >>>On Tue, 14 Oct 2014 22:55:50 -0400 Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > >>> > >>>>On Tue, Oct 14 2014 at 9:19pm -0400, > >>>>NeilBrown <neilb@xxxxxxx> 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@xxxxxxxx> > >>>>>Signed-off-by: NeilBrown <neilb@xxxxxxx> > >>>>> > >>>>>--- > >>>>>this issue has been discussed already a bit. See email thread > >>>>> Subject: Re: [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_. > > dm-raid uses 512 bytes for its superblock including padding in the > current code > (with way less payload), followed by the bitmap at offset 4096 bytes. > > With Neil's proposal, the padding will be avoided alltogether. > > So logical looks fine to me, because physical may well be more than > 4K in the future. > Admittingly a longer time in the future though. > > If that happens, we'd end up with the bitmap start at offset 4096 > bytes in the same physical sector > and would have to cope with it plus the issues involved with 4K page sizes. > > In order to prevent this to occur unrecognized in dm/md later, > we should have an "if (rdev->sb_size > PAGE_SIZE) return -EINVAL;" > after the setting > of rdev->sb_size in dm-raid.c and appropriate checks in md.c. > > BTW: even with additions to the dm-raid superblock I have to make in > order to allow for reshaping etc., > it's paylod is going to stay < 512 bytes. Heinz, If you could propose a revised patch I'd appreciate it. Thanks, Mike -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
