| Message ID | 44b2ec9c1acbaf8c0e13ef882e2340477bac379e.1615971432.git.johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: remove outdated WARN_ON in direct IO | expand |
On Wed, Mar 17, 2021 at 05:57:31PM +0900, Johannes Thumshirn wrote: > In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if > we're submitting a DIO write on a zoned filesystem but are not using > REQ_OP_ZONE_APPEND to submit the IO to the block device. > > This is a left over from a previous version where btrfs_dio_iomap_begin() > didn't use btrfs_use_zone_append() to check for sequential write only > zones. I can't identify the patch where this got changed. I've landed on 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO") but that adds the btrfs_use_zone_append, the append flag and also the warning.
On 17/03/2021 11:54, David Sterba wrote: > On Wed, Mar 17, 2021 at 05:57:31PM +0900, Johannes Thumshirn wrote: >> In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if >> we're submitting a DIO write on a zoned filesystem but are not using >> REQ_OP_ZONE_APPEND to submit the IO to the block device. >> >> This is a left over from a previous version where btrfs_dio_iomap_begin() >> didn't use btrfs_use_zone_append() to check for sequential write only >> zones. > > I can't identify the patch where this got changed. I've landed on > 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO") > but that adds the btrfs_use_zone_append, the append flag and also the > warning. > It is an oversight from the development phase. In v11 (I think) I've added 08f455593fff ("btrfs: zoned: cache if block group is on a sequential zone") and forgot to remove the WARN_ON_ONCE() for 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO"). When developing auto relocation I got hit by the WARN as a block groups where relocated to conventional zone and the dio code calls btrfs_use_zone_append() introduced by 08f455593fff to check if it can use zone append (a.k.a. if it's a sequential zone) or not and sets the appropriate flags for iomap. I've never hit it in testing before, as I was relying on emulation to test the conventional zones code but this one case wasn't hit, because on emulation fs_info->max_zone_append_size is 0 and the WARN doesn't trigger either.
On 17/03/2021 14:20, Johannes Thumshirn wrote: > On 17/03/2021 11:54, David Sterba wrote: >> On Wed, Mar 17, 2021 at 05:57:31PM +0900, Johannes Thumshirn wrote: >>> In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if >>> we're submitting a DIO write on a zoned filesystem but are not using >>> REQ_OP_ZONE_APPEND to submit the IO to the block device. >>> >>> This is a left over from a previous version where btrfs_dio_iomap_begin() >>> didn't use btrfs_use_zone_append() to check for sequential write only >>> zones. >> >> I can't identify the patch where this got changed. I've landed on >> 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO") >> but that adds the btrfs_use_zone_append, the append flag and also the >> warning. >> > > It is an oversight from the development phase. In v11 (I think) I've added > 08f455593fff ("btrfs: zoned: cache if block group is on a sequential zone") > and forgot to remove the WARN_ON_ONCE() for 544d24f9de73 ("btrfs: zoned: > enable zone append writing for direct IO"). > > When developing auto relocation I got hit by the WARN as a block groups > where relocated to conventional zone and the dio code calls > btrfs_use_zone_append() introduced by 08f455593fff to check if it can use > zone append (a.k.a. if it's a sequential zone) or not and sets the appropriate > flags for iomap. > > I've never hit it in testing before, as I was relying on emulation to test > the conventional zones code but this one case wasn't hit, because on emulation > fs_info->max_zone_append_size is 0 and the WARN doesn't trigger either. > I just realized that explanation should have gone into the commit message, do you want me to resend with a more elaborate commit message?
On Wed, Mar 17, 2021 at 01:22:11PM +0000, Johannes Thumshirn wrote: > On 17/03/2021 14:20, Johannes Thumshirn wrote: > > On 17/03/2021 11:54, David Sterba wrote: > >> On Wed, Mar 17, 2021 at 05:57:31PM +0900, Johannes Thumshirn wrote: > >>> In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if > >>> we're submitting a DIO write on a zoned filesystem but are not using > >>> REQ_OP_ZONE_APPEND to submit the IO to the block device. > >>> > >>> This is a left over from a previous version where btrfs_dio_iomap_begin() > >>> didn't use btrfs_use_zone_append() to check for sequential write only > >>> zones. > >> > >> I can't identify the patch where this got changed. I've landed on > >> 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO") > >> but that adds the btrfs_use_zone_append, the append flag and also the > >> warning. > >> > > > > It is an oversight from the development phase. In v11 (I think) I've added > > 08f455593fff ("btrfs: zoned: cache if block group is on a sequential zone") > > and forgot to remove the WARN_ON_ONCE() for 544d24f9de73 ("btrfs: zoned: > > enable zone append writing for direct IO"). > > > > When developing auto relocation I got hit by the WARN as a block groups > > where relocated to conventional zone and the dio code calls > > btrfs_use_zone_append() introduced by 08f455593fff to check if it can use > > zone append (a.k.a. if it's a sequential zone) or not and sets the appropriate > > flags for iomap. > > > > I've never hit it in testing before, as I was relying on emulation to test > > the conventional zones code but this one case wasn't hit, because on emulation > > fs_info->max_zone_append_size is 0 and the WARN doesn't trigger either. > > I just realized that explanation should have gone into the commit message, do you > want me to resend with a more elaborate commit message? I'll append the explanation above to the changelog, no need to resend unless you want to add/adjust something. Thanks.
On 17/03/2021 15:29, David Sterba wrote: > On Wed, Mar 17, 2021 at 01:22:11PM +0000, Johannes Thumshirn wrote: >> On 17/03/2021 14:20, Johannes Thumshirn wrote: >>> On 17/03/2021 11:54, David Sterba wrote: >>>> On Wed, Mar 17, 2021 at 05:57:31PM +0900, Johannes Thumshirn wrote: >>>>> In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if >>>>> we're submitting a DIO write on a zoned filesystem but are not using >>>>> REQ_OP_ZONE_APPEND to submit the IO to the block device. >>>>> >>>>> This is a left over from a previous version where btrfs_dio_iomap_begin() >>>>> didn't use btrfs_use_zone_append() to check for sequential write only >>>>> zones. >>>> >>>> I can't identify the patch where this got changed. I've landed on >>>> 544d24f9de73 ("btrfs: zoned: enable zone append writing for direct IO") >>>> but that adds the btrfs_use_zone_append, the append flag and also the >>>> warning. >>>> >>> >>> It is an oversight from the development phase. In v11 (I think) I've added >>> 08f455593fff ("btrfs: zoned: cache if block group is on a sequential zone") >>> and forgot to remove the WARN_ON_ONCE() for 544d24f9de73 ("btrfs: zoned: >>> enable zone append writing for direct IO"). >>> >>> When developing auto relocation I got hit by the WARN as a block groups >>> where relocated to conventional zone and the dio code calls >>> btrfs_use_zone_append() introduced by 08f455593fff to check if it can use >>> zone append (a.k.a. if it's a sequential zone) or not and sets the appropriate >>> flags for iomap. >>> >>> I've never hit it in testing before, as I was relying on emulation to test >>> the conventional zones code but this one case wasn't hit, because on emulation >>> fs_info->max_zone_append_size is 0 and the WARN doesn't trigger either. >> >> I just realized that explanation should have gone into the commit message, do you >> want me to resend with a more elaborate commit message? > > I'll append the explanation above to the changelog, no need to resend > unless you want to add/adjust something. Thanks. > Thanks
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 618ec195985b..288c7ce63a32 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8179,10 +8179,6 @@ static blk_qc_t btrfs_submit_direct(struct inode *inode, struct iomap *iomap, bio->bi_end_io = btrfs_end_dio_bio; btrfs_io_bio(bio)->logical = file_offset; - WARN_ON_ONCE(write && btrfs_is_zoned(fs_info) && - fs_info->max_zone_append_size && - bio_op(bio) != REQ_OP_ZONE_APPEND); - if (bio_op(bio) == REQ_OP_ZONE_APPEND) { status = extract_ordered_extent(BTRFS_I(inode), bio, file_offset);
In btrfs_submit_direct() there's a WAN_ON_ONCE() that will trigger if we're submitting a DIO write on a zoned filesystem but are not using REQ_OP_ZONE_APPEND to submit the IO to the block device. This is a left over from a previous version where btrfs_dio_iomap_begin() didn't use btrfs_use_zone_append() to check for sequential write only zones. Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/inode.c | 4 ---- 1 file changed, 4 deletions(-)