Re: [PATCH v3 2/5] btrfs: remove use of buffer_heads from superblock writeout

Christoph Hellwig
2020-01-28T11:52:42Z

On Tue, Jan 28, 2020 at 12:59:28AM +0900, Johannes Thumshirn wrote:
> Similar to the superblock read path, change the write path to using BIOs
> and pages instead of buffer_heads. This allows us to skip over the
> buffer_head code, for writing the superblock to disk.

Hmm, the previous patch already changed one place that wrote the sb..

> - /* One reference for us, and we leave it for the caller */
> - bh = __getblk(device->bdev, bytenr / BTRFS_BDEV_BLOCKSIZE,
> - BTRFS_SUPER_INFO_SIZE);
> - if (!bh) {
> + page = find_or_create_page(mapping, bytenr >> PAGE_SHIFT,
> + gfp_mask);
> + if (!page) {
> btrfs_err(device->fs_info,
> - "couldn't get super buffer head for bytenr %llu",
> + "couldn't get superblock page for bytenr %llu",
> bytenr);
> errors++;
> continue;
> }
>
> - memcpy(bh->b_data, sb, BTRFS_SUPER_INFO_SIZE);

> + /* Bump the refcount for wait_dev_supers() */
> + get_page(page);
>
> - /* one reference for submit_bh */
> - get_bh(bh);
> -
> - set_buffer_uptodate(bh);
> - lock_buffer(bh);
> - bh->b_end_io = btrfs_end_buffer_write_sync;
> - bh->b_private = device;
> + ptr = kmap(page);
> + memcpy(ptr, sb, BTRFS_SUPER_INFO_SIZE);
> + kunmap(page);

What is the point of using the page cache here given that you are
always using a local copy of the data anyway?

> + wait_on_page_locked(page);
> + if (PageError(page)) {
> errors++;
> if (i == 0)
> primary_failed = true;
> }
>
> /* drop our reference */
> - brelse(bh);
> + put_page(page);
>
> /* drop the reference from the writing run */
> - brelse(bh);
> + put_page(page);

Why not use an inflight count + completion instead of messing
with the page lock like that?