Re: [PATCH v7 02/13] btrfs: introduce a new allocator for scrub specific btrfs_bio

[Christoph Hellwig <hch@infradead.org>]

> +struct btrfs_bio *btrfs_scrub_bio_alloc(blk_opf_t opf,
> +					struct btrfs_fs_info *fs_info,
> +					btrfs_bio_end_io_t end_io, void *private)
> +{
> +	struct btrfs_bio *bbio;
> +	struct bio *bio;
> +
> +	bio = bio_alloc_bioset(NULL, BTRFS_STRIPE_LEN >> fs_info->sectorsize_bits,
> +			       opf, GFP_NOFS, &btrfs_bioset);
> +	bbio = btrfs_bio(bio);
> +	memset(bbio, 0, offsetof(struct btrfs_bio, bio));
> +	bbio->fs_info = fs_info;
> +	bbio->end_io = end_io;
> +	bbio->private = private;
> +	atomic_set(&bbio->pending_ios, 1);
> +	return bbio;

As mentioned in the last round, I'm not too happy with this.  With
the inode and file_offset being optional now we might as well drop them
as arguments from btrfs_bio_alloc/btrfs_bio_init and just pass a nr_vecs
instead and make this new allocator obsolete, and it would be a much
better result.

I'd prefer to just do it now rather than doing another series changing
it a little later.

> +	/*
> +	 * Inode and offset into it that this I/O operates on.
> +	 *
> +	 * @inode can be NULL for callers who don't want any advanced features
> +	 * like read-time repair.
> +	 */
>  	struct btrfs_inode *inode;
>  	u64 file_offset;

I don't think these negative comments are nice for the reader.  I'd do:

	/*
	 * Inode and offset into it that this I/O operates on.
	 * Only set for data I/O.
	 */

> +	/*
> +	 * For cases where callers only want to read/write from a logical
> +	 * bytenr, in that case @inode can be NULL, and we need such
> +	 * @fs_info pointer to grab the corresponding fs_info.
> +	 *
> +	 * Should always be populated.
> +	 */
> +	struct btrfs_fs_info *fs_info;

Again here, this comment only makes sense for people following the
development history of this particular patch series.  Once that is in
the reason why people use an inode before is irrelevant.  The only
useful bit left here is that it must always be populated, but I'm not
even sure I'd add that.  So all we might need is:

	/* File system that this I/O operates on. */

What would be good in this patch is to replace the
existing bbio->inode->root->fs_info dereferences with bbio->fs_info
ASAP, though.