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.
next prev parent reply other threads:[~2023-03-23 22:43 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-22 19:11 [PATCH v5 0/5] btrfs: fix corruption caused by partial dio writes Boris Burkov
2023-03-22 19:11 ` [PATCH v5 1/5] btrfs: add function to create and return an ordered extent Boris Burkov
2023-03-22 19:11 ` [PATCH v5 2/5] btrfs: stash ordered extent in dio_data during iomap dio Boris Burkov
2023-03-22 19:11 ` [PATCH v5 3/5] btrfs: return ordered_extent splits from bio extraction Boris Burkov
2023-03-23 8:47 ` Christoph Hellwig
2023-03-23 16:15 ` Boris Burkov
2023-03-23 17:00 ` Boris Burkov
2023-03-23 17:45 ` Boris Burkov
2023-03-23 21:29 ` Christoph Hellwig
2023-03-23 22:43 ` Boris Burkov [this message]
2023-03-24 0:24 ` Christoph Hellwig
2023-03-22 19:11 ` [PATCH v5 4/5] btrfs: fix crash with non-zero pre in btrfs_split_ordered_extent Boris Burkov
2023-03-23 8:36 ` Naohiro Aota
2023-03-23 16:22 ` Boris Burkov
2023-03-22 19:11 ` [PATCH v5 5/5] btrfs: split partial dio bios before submit Boris Burkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230323224336.GA29323@zen \
--to=boris@bur.io \
--cc=hch@infradead.org \
--cc=kernel-team@fb.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).