btrfs zoned fixlets

btrfs zoned fixlets

[Christoph Hellwig]

Hi all,

this series fixes a minor and slightly less minor problem in the btrfs
zoned device code.  Note that for the second patch the comment might
not be correct any more - AFAICS 5.18 added support for the dup
profile for zoned devices, which means we do have a real issue now
if different devices have different hardware limitations.  I think we'll
need some code to check that all zoned devices have the exact same
hardware limits (max_sectors, max_segments, max_segment_size,
queue_boundary, virt_boundary), but I don't know the code well enough
to implement that myself

Found by code inspection as part of my bio cleanups.

[PATCH 1/2] btrfs: remove the zoned/zone_size union in struct btrfs_fs_info

[Christoph Hellwig]

Reading a value from a different member of a union is not just a great
way to obsfucate code, but also creates an aliasing violation.  Switch
btrfs_is_zoned to look at ->zone_size and remove the union.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/ctree.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index b7631b88426e3..0edcce6d2db64 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1045,10 +1045,7 @@ struct btrfs_fs_info {
 	 * Zone size > 0 when in ZONED mode, otherwise it's used for a check
 	 * if the mode is enabled
 	 */
-	union {
-		u64 zone_size;
-		u64 zoned;
-	};
+	u64 zone_size;
 
 	struct mutex zoned_meta_io_lock;
 	spinlock_t treelog_bg_lock;
@@ -3928,7 +3925,7 @@ static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
 
 static inline bool btrfs_is_zoned(const struct btrfs_fs_info *fs_info)
 {
-	return fs_info->zoned != 0;
+	return fs_info->zone_size > 0;
 }
 
 static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
-- 
2.30.2

Re: [PATCH 1/2] btrfs: remove the zoned/zone_size union in struct btrfs_fs_info

[Johannes Thumshirn]

Fine by me, but it was explicitly requested to be this way IIRC.

Anyways,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/2] btrfs: remove the zoned/zone_size union in struct btrfs_fs_info

[David Sterba]

On Fri, Mar 25, 2022 at 07:33:25AM +0000, Johannes Thumshirn wrote:
> Fine by me, but it was explicitly requested to be this way IIRC.

This was before there was the btrfs_is_zoned helper that now wraps the
condition.

Re: [PATCH 1/2] btrfs: remove the zoned/zone_size union in struct btrfs_fs_info

[Naohiro Aota]

On Thu, Mar 24, 2022 at 05:52:09PM +0100, Christoph Hellwig wrote:
> Reading a value from a different member of a union is not just a great
> way to obsfucate code, but also creates an aliasing violation.  Switch
> btrfs_is_zoned to look at ->zone_size and remove the union.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Also, fine for me.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

Re: [PATCH 1/2] btrfs: remove the zoned/zone_size union in struct btrfs_fs_info

[David Sterba]

On Thu, Mar 24, 2022 at 05:52:09PM +0100, Christoph Hellwig wrote:
> Reading a value from a different member of a union is not just a great
> way to obsfucate code, but also creates an aliasing violation.

Is it a violation in this case? Both are of the same type, violation
could be int/u64 and others but even in this case this is transparent to
the compiler and union cast is the cleanest way to access same bytes in
a structure. Anaywy now the helper btrfs_is_zoned is used everywhere
which abstracts the condition, which was the original idea for the
union.

[PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[Christoph Hellwig]

Zone Append bios only need a valid block device in struct bio, but
not the device in the btrfs_bio.  Use the information from
btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
multi-device file system with non-homogeneous capabilities and remove
the pointless btrfs_bio.device assignment.

Add big fat comments explaining what is going on here.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/extent_io.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d78b3a2d04e3b..175a3f16c2da0 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3334,24 +3334,37 @@ static int alloc_new_bio(struct btrfs_inode *inode,
 	ret = calc_bio_boundaries(bio_ctrl, inode, file_offset);
 	if (ret < 0)
 		goto error;
-	if (wbc) {
-		struct block_device *bdev;
 
-		bdev = fs_info->fs_devices->latest_dev->bdev;
-		bio_set_dev(bio, bdev);
-		wbc_init_bio(wbc, bio);
-	}
-	if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
-		struct btrfs_device *device;
+	if (wbc) {
+		/*
+		 * For Zone append we need the correct block_device that we are
+		 * going to write to set in the bio to be able to respect the
+		 * hardware limitation.  Look it up here:
+		 */
+		if (bio_op(bio) == REQ_OP_ZONE_APPEND) {
+			struct btrfs_device *dev;
+
+			dev = btrfs_zoned_get_device(fs_info, disk_bytenr,
+						     fs_info->sectorsize);
+			if (IS_ERR(dev)) {
+				ret = PTR_ERR(dev);
+				goto error;
+			}
 
-		device = btrfs_zoned_get_device(fs_info, disk_bytenr,
-						fs_info->sectorsize);
-		if (IS_ERR(device)) {
-			ret = PTR_ERR(device);
-			goto error;
+			bio_set_dev(bio, dev->bdev);
+		} else {
+			/*
+			 * Otherwise pick the last added device to support
+			 * cgroup writeback.  For multi-device file systems this
+			 * means blk-cgroup policies have to always be set on the
+			 * last added/replaced device.  This is a bit odd but has
+			 * been like that for a long time.
+			 */
+			bio_set_dev(bio, fs_info->fs_devices->latest_dev->bdev);
 		}
-
-		btrfs_bio(bio)->device = device;
+		wbc_init_bio(wbc, bio);
+	} else {
+		ASSERT(bio_op(bio) != REQ_OP_ZONE_APPEND);
 	}
 	return 0;
 error:
-- 
2.30.2

Re: [PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[Johannes Thumshirn]

On 24/03/2022 17:54, Christoph Hellwig wrote:
> Zone Append bios only need a valid block device in struct bio, but
> not the device in the btrfs_bio.  Use the information from
> btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
> multi-device file system with non-homogeneous capabilities and remove
> the pointless btrfs_bio.device assignment.
> 
> Add big fat comments explaining what is going on here.

Looks like the old code worked by sheer luck, as we had wbc set and thus
always assigned fs_info->fs_devices->latest_dev->bdev to the bio. Which 
would obviously not work on a multi device FS.


Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[David Sterba]

On Fri, Mar 25, 2022 at 09:09:56AM +0000, Johannes Thumshirn wrote:
> On 24/03/2022 17:54, Christoph Hellwig wrote:
> > Zone Append bios only need a valid block device in struct bio, but
> > not the device in the btrfs_bio.  Use the information from
> > btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
> > multi-device file system with non-homogeneous capabilities and remove
> > the pointless btrfs_bio.device assignment.
> > 
> > Add big fat comments explaining what is going on here.
> 
> Looks like the old code worked by sheer luck, as we had wbc set and thus
> always assigned fs_info->fs_devices->latest_dev->bdev to the bio. Which 
> would obviously not work on a multi device FS.

No, it worked fine because the real bio is set just before writing the
data somewhere deep in the io submit path in submit_stripe_bio().

That it has to be set here is because of the cgroup implementation that
accesses it, see 429aebc0a9a0 ("btrfs: get bdev directly from fs_devices
in submit_extent_page").

Which brings me to the question if Christoph's fix is correct because
the comment for the wbc + zoned append is assuming something that's not
true.

Re: [PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[Naohiro Aota]

On Mon, Mar 28, 2022 at 09:12:40PM +0200, David Sterba wrote:
> On Fri, Mar 25, 2022 at 09:09:56AM +0000, Johannes Thumshirn wrote:
> > On 24/03/2022 17:54, Christoph Hellwig wrote:
> > > Zone Append bios only need a valid block device in struct bio, but
> > > not the device in the btrfs_bio.  Use the information from
> > > btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
> > > multi-device file system with non-homogeneous capabilities and remove
> > > the pointless btrfs_bio.device assignment.
> > > 
> > > Add big fat comments explaining what is going on here.
> > 
> > Looks like the old code worked by sheer luck, as we had wbc set and thus
> > always assigned fs_info->fs_devices->latest_dev->bdev to the bio. Which 
> > would obviously not work on a multi device FS.
> 
> No, it worked fine because the real bio is set just before writing the
> data somewhere deep in the io submit path in submit_stripe_bio().
> 
> That it has to be set here is because of the cgroup implementation that
> accesses it, see 429aebc0a9a0 ("btrfs: get bdev directly from fs_devices
> in submit_extent_page").
> 
> Which brings me to the question if Christoph's fix is correct because
> the comment for the wbc + zoned append is assuming something that's not
> true.

While the real bio is setup in submit_stripe_bio(), we need to set the
device destination for bio_add_zone_append_page() called in
btrfs_bio_add_page(). The bio_add_zone_append_page() checks that the
bio length is not exceeding max_zone_append_sectors() of the device,
and checks other hardware restrictions.

Re: [PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[David Sterba]

On Mon, Mar 28, 2022 at 11:04:26PM +0000, Naohiro Aota wrote:
> On Mon, Mar 28, 2022 at 09:12:40PM +0200, David Sterba wrote:
> > On Fri, Mar 25, 2022 at 09:09:56AM +0000, Johannes Thumshirn wrote:
> > > On 24/03/2022 17:54, Christoph Hellwig wrote:
> > > > Zone Append bios only need a valid block device in struct bio, but
> > > > not the device in the btrfs_bio.  Use the information from
> > > > btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
> > > > multi-device file system with non-homogeneous capabilities and remove
> > > > the pointless btrfs_bio.device assignment.
> > > > 
> > > > Add big fat comments explaining what is going on here.
> > > 
> > > Looks like the old code worked by sheer luck, as we had wbc set and thus
> > > always assigned fs_info->fs_devices->latest_dev->bdev to the bio. Which 
> > > would obviously not work on a multi device FS.
> > 
> > No, it worked fine because the real bio is set just before writing the
> > data somewhere deep in the io submit path in submit_stripe_bio().
> > 
> > That it has to be set here is because of the cgroup implementation that
> > accesses it, see 429aebc0a9a0 ("btrfs: get bdev directly from fs_devices
> > in submit_extent_page").
> > 
> > Which brings me to the question if Christoph's fix is correct because
> > the comment for the wbc + zoned append is assuming something that's not
> > true.
> 
> While the real bio is setup in submit_stripe_bio(), we need to set the

Oh sorry I actually wanted to say that the real 'bdev' is set in
submit_stripe_bio (ie. the one where the write is going to be done).

> device destination for bio_add_zone_append_page() called in
> btrfs_bio_add_page(). The bio_add_zone_append_page() checks that the
> bio length is not exceeding max_zone_append_sectors() of the device,
> and checks other hardware restrictions.

Yeah, but can this still mean that it's checking potentially different
devices with different hw restrictions? In alloc_new_bio() it's one and in
submit_stripe_bio() it's a different one.

Before the cgroup writeback was added to bios, the only reason why
bio_set_bdev required the block device is to check if it's the same one
as before and drop some bit:

static inline void bio_set_dev(struct bio *bio, struct block_device *bdev)
{
	bio_clear_flag(bio, BIO_REMAPPED);
	if (bio->bi_bdev != bdev)
		bio_clear_flag(bio, BIO_THROTTLED);
	bio->bi_bdev = bdev;
	bio_associate_blkg(bio);		<-- this was not here
}

So the latest_dev was just a stub to satisfy the bio API requirements.
Please note that its existence spans a long time and things have
changed, I remember that Chris' answer to why we need the latest_dev was
"to put something to the bios". Ie. we don't need it because we have to
write same data to different block devices and distribute that in
submit_stripe_bio(), while the bios have to be set much earlier
expecting a block device.

I'm not sure we have a 1:1 match in what the APIs provide and expect and
what btrfs wants to do. At this point multi-device support for zoned
mode is not complete so we probably won't observe any problems with
hardware with different restrictions.

Re: [PATCH 2/2] btrfs: fix and document the zoned device choice in alloc_new_bio

[Naohiro Aota]

On Thu, Mar 24, 2022 at 05:52:10PM +0100, Christoph Hellwig wrote:
> Zone Append bios only need a valid block device in struct bio, but
> not the device in the btrfs_bio.  Use the information from
> btrfs_zoned_get_device to set up bi_bdev and fix zoned writes on
> multi-device file system with non-homogeneous capabilities and remove
> the pointless btrfs_bio.device assignment.
> 
> Add big fat comments explaining what is going on here.

Looks good to me.

Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>

I'm wondering why I used the ->device here. Maybe, it was left over
from pre-btrfs_bio_add_page().

Re: btrfs zoned fixlets

[Johannes Thumshirn]

On 24/03/2022 17:55, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes a minor and slightly less minor problem in the btrfs
> zoned device code.  Note that for the second patch the comment might
> not be correct any more - AFAICS 5.18 added support for the dup
> profile for zoned devices, which means we do have a real issue now
> if different devices have different hardware limitations.

It's not a problem, as a) DUP is not spanning multiple devices, but
only used on one device and b) DUP is (as of now) only used on meta-data
which can't use zone append.

Re: btrfs zoned fixlets

[Christoph Hellwig]

On Fri, Mar 25, 2022 at 07:35:57AM +0000, Johannes Thumshirn wrote:
> On 24/03/2022 17:55, Christoph Hellwig wrote:
> > Hi all,
> > 
> > this series fixes a minor and slightly less minor problem in the btrfs
> > zoned device code.  Note that for the second patch the comment might
> > not be correct any more - AFAICS 5.18 added support for the dup
> > profile for zoned devices, which means we do have a real issue now
> > if different devices have different hardware limitations.
> 
> It's not a problem, as a) DUP is not spanning multiple devices, but
> only used on one device and b) DUP is (as of now) only used on meta-data
> which can't use zone append.

Ok, in that case the comment that I wrote based on the 5.17-ish code
is still correct.

Re: btrfs zoned fixlets

[Christoph Hellwig]

David, 

do you plan to pick these up?

Re: btrfs zoned fixlets

[David Sterba]

On Fri, Apr 08, 2022 at 06:41:01PM +0200, Christoph Hellwig wrote:
> David, 
> 
> do you plan to pick these up?

Yes I do but I had some questions after the initial review and don't
know when I'll get back to it because of other patches being reviewed.

Re: btrfs zoned fixlets

[David Sterba]

On Thu, Mar 24, 2022 at 05:52:08PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series fixes a minor and slightly less minor problem in the btrfs
> zoned device code.  Note that for the second patch the comment might
> not be correct any more - AFAICS 5.18 added support for the dup
> profile for zoned devices, which means we do have a real issue now
> if different devices have different hardware limitations.  I think we'll
> need some code to check that all zoned devices have the exact same
> hardware limits (max_sectors, max_segments, max_segment_size,
> queue_boundary, virt_boundary), but I don't know the code well enough
> to implement that myself
> 
> Found by code inspection as part of my bio cleanups.

Added to misc-next, thanks.