From: Naohiro Aota <Naohiro.Aota@wdc.com>
To: Filipe Manana <fdmanana@kernel.org>
Cc: "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH 3/4] btrfs: fix error handling of fallbacked uncompress write
Date: Mon, 20 Jun 2022 02:26:44 +0000 [thread overview]
Message-ID: <20220620022644.owk5kbqrtxj4x6e5@naota-xeon> (raw)
In-Reply-To: <20220617102144.GD4041436@falcondesktop>
On Fri, Jun 17, 2022 at 11:21:44AM +0100, Filipe Manana wrote:
> On Fri, Jun 17, 2022 at 12:45:41AM +0900, Naohiro Aota wrote:
> > When cow_file_range() fails in the middle of the allocation loop, it
> > unlocks the pages but remains the ordered extents intact. Thus, we need to
>
> s/remains/leaves/
Will fix.
>
> > call btrfs_cleanup_ordered_extents() to finish the created ordered extents.
> >
> > Also, we need to call end_extent_writepage() if locked_page is available
> > because btrfs_cleanup_ordered_extents() never process the region on the
> > locked_page.
> >
> > Furthermore, we need to set the mapping as error if locked_page is
> > unavailable before unlocking the pages, so that the errno is properly
> > propagated to the userland.
> >
> > CC: stable@vger.kernel.org
>
> It would be better to specify a version here.
>
> The delalloc paths for compression were (a bit heavily) refactored last year
> in preparation for the subpage sector size support, so blindly adding this
> to any stable releases might introduce regressions (assuming the patch does
> not fail to apply).
Yeah ... I could not find the exact commit. It looks like there is no
ordered extent clean up from the beginning.
I'll check if the modified cow_file_range() cause a hang on the stable
versions.
>
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> > fs/btrfs/inode.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 4e1100f84a88..cae15924fc99 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -934,8 +934,18 @@ static int submit_uncompressed_range(struct btrfs_inode *inode,
> > goto out;
> > }
> > if (ret < 0) {
> > - if (locked_page)
> > + btrfs_cleanup_ordered_extents(inode, locked_page, start, end - start + 1);
> > + if (locked_page) {
> > + u64 page_start = page_offset(locked_page);
> > + u64 page_end = page_start + PAGE_SIZE - 1;
> > +
> > + btrfs_page_set_error(inode->root->fs_info, locked_page,
> > + page_start, PAGE_SIZE);
> > + set_page_writeback(locked_page);
> > + end_page_writeback(locked_page);
> > + end_extent_writepage(locked_page, ret, page_start, page_end);
> > unlock_page(locked_page);
> > + }
> > goto out;
> > }
>
> So as I commented in the previous patch, something is missing here: the call to
> btrfs_cleanup_ordered_extents() at submit_uncompressed_range() in case cow_file_range()
> returns an error.
>
> Otherwise it looks fine.
> Thanks.
btrfs_cleanup_ordered_extents() is called at the first line of the added
lines. We need to call it regardless of locked_page != NULL or not, because
submit_uncompressed_range() can be called with locked_page = NULL.
> >
> > @@ -1390,9 +1400,12 @@ static noinline int cow_file_range(struct btrfs_inode *inode,
> > * However, in case of unlock == 0, we still need to unlock the pages
> > * (except @locked_page) to ensure all the pages are unlocked.
> > */
> > - if (!unlock && orig_start < start)
> > + if (!unlock && orig_start < start) {
> > + if (!locked_page)
> > + mapping_set_error(inode->vfs_inode.i_mapping, ret);
> > extent_clear_unlock_delalloc(inode, orig_start, start - 1,
> > locked_page, 0, page_ops);
> > + }
> >
> > /*
> > * For the range (2). If we reserved an extent for our delalloc range
> > --
> > 2.35.1
> >
next prev parent reply other threads:[~2022-06-20 2:26 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-16 15:45 [PATCH 0/4] btrfs: fix error handling of cow_file_range(unlock = 0) Naohiro Aota
2022-06-16 15:45 ` [PATCH 1/4] btrfs: ensure pages are unlocked on cow_file_range() failure Naohiro Aota
2022-06-17 10:11 ` Filipe Manana
2022-06-20 2:22 ` Naohiro Aota
2022-06-16 15:45 ` [PATCH 2/4] btrfs: extend btrfs_cleanup_ordered_extens for NULL locked_page Naohiro Aota
2022-06-17 10:15 ` Filipe Manana
2022-06-20 2:28 ` Naohiro Aota
2022-06-16 15:45 ` [PATCH 3/4] btrfs: fix error handling of fallbacked uncompress write Naohiro Aota
2022-06-17 10:21 ` Filipe Manana
2022-06-20 2:26 ` Naohiro Aota [this message]
2022-06-16 15:45 ` [PATCH 4/4] btrfs: replace unnecessary goto with direct return Naohiro Aota
2022-06-17 10:13 ` Filipe Manana
2022-06-20 2:22 ` Naohiro Aota
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=20220620022644.owk5kbqrtxj4x6e5@naota-xeon \
--to=naohiro.aota@wdc.com \
--cc=fdmanana@kernel.org \
--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).