lkml.org 
[lkml]   [2019]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/2] [PATCH] bcache: __write_super to handle page sizes other than 4k
    Sure. will do in a follow up patch.

    On Fri, Dec 6, 2019 at 5:44 PM Coly Li <colyli@suse.de> 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@gmail.com>
    >
    > 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

    \
     
     \ /
      Last update: 2019-12-06 12:23    [W:4.148 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site