Re: [PATCH 2/2] [PATCH] bcache: __write_super to handle page sizes other than 4k

[Liang C]

Sure. will do in a follow up patch.

On Fri, Dec 6, 2019 at 5:44 PM Coly Li <colyli@xxxxxxx> wrote:
>
> On 2019/12/6 4:55 äå, Liang Chen wrote:
> > __write_super assumes super block data starts at offset 0 of the page
> > read in with __bread from read_super, which is not true when page size
> > is not 4k. We encountered the issue on system with 64K page size - commonly
> >  seen on aarch64 architecture.
> >
> > Instead of making any assumption on the offset of the data within the page,
> > this patch calls __bread again to locate the data. That should not introduce
> > an extra io since the page has been held when it's read in from read_super,
> > and __write_super is not on performance critical code path.
> >
> > Signed-off-by: Liang Chen <liangchen.linux@xxxxxxxxx>
>
> In general the patch is good for me. Just two minor requests I add them
> in line the email.
>
> Thanks.
>
> > ---
> >  drivers/md/bcache/super.c | 32 +++++++++++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index a573ce1d85aa..a39450c9bc34 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -207,15 +207,27 @@ static void write_bdev_super_endio(struct bio *bio)
> >       closure_put(&dc->sb_write);
> >  }
> >
> > -static void __write_super(struct cache_sb *sb, struct bio *bio)
> > +static int __write_super(struct cache_sb *sb, struct bio *bio,
> > +                      struct block_device *bdev)
> >  {
> > -     struct cache_sb *out = page_address(bio_first_page_all(bio));
> > +     struct cache_sb *out;
> >       unsigned int i;
> > +     struct buffer_head *bh;
> > +
> > +     /*
> > +      * The page is held since read_super, this __bread * should not
> > +      * cause an extra io read.
> > +      */
> > +     bh = __bread(bdev, 1, SB_SIZE);
> > +     if (!bh)
> > +             goto out_bh;
> > +
> > +     out = (struct cache_sb *) bh->b_data;
>
> This is quite tricky here. Could you please to move this code piece into
> an inline function and add code comments to explain why a read is
> necessary for a write.
>
>
> >
> >       bio->bi_iter.bi_sector  = SB_SECTOR;
> >       bio->bi_iter.bi_size    = SB_SIZE;
> >       bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_SYNC|REQ_META);
> > -     bch_bio_map(bio, NULL);
> > +     bch_bio_map(bio, bh->b_data);
> >
> >       out->offset             = cpu_to_le64(sb->offset);
> >       out->version            = cpu_to_le64(sb->version);
> > @@ -239,7 +251,14 @@ static void __write_super(struct cache_sb *sb, struct bio *bio)
> >       pr_debug("ver %llu, flags %llu, seq %llu",
> >                sb->version, sb->flags, sb->seq);
> >
> > +     /* The page will still be held without this bh.*/
> > +     put_bh(bh);
> >       submit_bio(bio);
> > +     return 0;
> > +
> > +out_bh:
> > +     pr_err("Couldn't read super block, __write_super failed");
> > +     return -1;
> >  }
> >
> >  static void bch_write_bdev_super_unlock(struct closure *cl)
> > @@ -264,7 +283,8 @@ void bch_write_bdev_super(struct cached_dev *dc, struct closure *parent)
> >
> >       closure_get(cl);
> >       /* I/O request sent to backing device */
> > -     __write_super(&dc->sb, bio);
> > +     if(__write_super(&dc->sb, bio, dc->bdev))
> > +             closure_put(cl);
> >
> >       closure_return_with_destructor(cl, bch_write_bdev_super_unlock);
> >  }
> > @@ -312,7 +332,9 @@ void bcache_write_super(struct cache_set *c)
> >               bio->bi_private = ca;
> >
> >               closure_get(cl);
> > -             __write_super(&ca->sb, bio);
> > +             if(__write_super(&ca->sb, bio, ca->bdev))
>
> And here, please add code comments for why closure_put() is necessary here.
>
> > +                     closure_put(cl);
> > +
> >       }
> >
> >       closure_return_with_destructor(cl, bcache_write_super_unlock);
> >
>
>
> --
>
> Coly Li