Re: [PATCH v5 3/5] btrfs: return ordered_extent splits from bio extraction

From: Boris Burkov <boris@bur.io>
To: Christoph Hellwig <hch@infradead.org>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH v5 3/5] btrfs: return ordered_extent splits from bio extraction
Date: Thu, 23 Mar 2023 15:43:36 -0700	[thread overview]
Message-ID: <20230323224336.GA29323@zen> (raw)
In-Reply-To: <ZBzEw4gy8lhNYgo7@infradead.org>

On Thu, Mar 23, 2023 at 02:29:39PM -0700, Christoph Hellwig wrote:
> On Thu, Mar 23, 2023 at 10:00:06AM -0700, Boris Burkov wrote:
> > Your branch as-is does not pass the existing tests, It's missing a fix
> > from my V5. We need to avoid splitting partial OEs when doing NOCOW dio
> > writes, because iomap_begin() does not create a fresh pinned em in that
> > case, since it reuses the existing extent.
> 
> Oops, yes, that got lost.  I can add this as another patch attributed
> to you.
> 
> That beeing said, I'm a bit confused about:
> 
>  1) if we need this split call at all for the non-zoned case as we don't
>     need to record a different physical disk address

I think I understand this, but maybe I'm missing exactly what you're
asking.

In finish_ordered_io, we call unpin_extent_cache, which blows up on
em->start != oe->file_offset. I believe the rationale is we are creating
a new em which is PINNED when we allocate the extent in
btrfs_new_extent_direct (via the call to btrfs_reserve_extent), so we
need to unpin it and allow it to be merged, etc... For nocow, we don't
allocate that new extent, so we don't need to split/unpin the existing
extent_map which we are just reusing.

>  2) how we clean up this on-disk logical to physical mapping at all on
>     a write failure

This I haven't thought much about, so I will leave it in the "dragons
sleep for now" category.

> 
> Maybe we should let those dragons sleep for now and just do the minimal
> fix, though.
> 
> I just woke up on an airplane, so depending on my jetlag I might have
> a new series ready with the minimal fix for varying definitions of
> "in a few hours".

Great, that works for me. I just didn't want to wait weeks if you were
blocked on other stuff.

> 
> > 
> > e.g.,
> > 
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 8cb61f4daec0..bbc89a0872e7 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7719,7 +7719,7 @@ static void btrfs_dio_submit_io(const struct iomap_iter *iter, struct bio *bio,
> >          * cancelled in iomap_end to avoid a deadlock wherein faulting the
> >          * remaining pages is blocked on the outstanding ordered extent.
> >          */
> > -       if (iter->flags & IOMAP_WRITE) {
> > +       if (iter->flags & IOMAP_WRITE && !test_bit(BTRFS_ORDERED_NOCOW, &dio_data->ordered->flags)) {
> >                 int err;
> > 
> >                 err = btrfs_extract_ordered_extent(bbio, dio_data->ordered);
> 
> I think the BTRFS_ORDERED_NOCOW should be just around the
> split_extent_map call.  That matches your series, and without
> that we wouldn't split the ordered_extent for nowcow writes and thus
> only fix the original problem for non-nocow writes.

Oops, my bad. Good catch.