From: Christoph Hellwig <hch@lst.de>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>,
chandanbabu@kernel.org, david@fromorbit.com,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH] xfs: fix error returns from xfs_bmapi_write
Date: Fri, 5 Apr 2024 19:00:42 +0200 [thread overview]
Message-ID: <20240405170042.GA15760@lst.de> (raw)
In-Reply-To: <20240405160742.GB6390@frogsfrogsfrogs>
On Fri, Apr 05, 2024 at 09:07:42AM -0700, Darrick J. Wong wrote:
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -321,14 +321,6 @@ xfs_iomap_write_direct(
> > if (error)
> > goto out_unlock;
> >
> > - /*
> > - * Copy any maps to caller's array and return any error.
> > - */
> > - if (nimaps == 0) {
> > - error = -ENOSPC;
> > - goto out_unlock;
> > - }
>
> Can xfs_bmapi_write return ENOSR here such that it leaks out to
> userspace?
It shouldn't because we invalidated all pages before, although I guess
if we race badly enough with page_mkwrite new a new delalloc region
could have been created and merged with one before the write and then if
the file system was fragmented to death we could hit the case where
we hit ENOSR now (and the wrong ENOSPC before).
That being said handling the error everywhere doesn't really scale.
I've actually looked into not converting the entire delalloc extent
(which btw is behavior that goes back to the initial commit of delalloc
support in XFS), but that quickly ran into asserts in the low-level
xfs_bmap.c. I plan to get back into it because it feels like the
right thing to do outside of the buffered writeback code, and whatever
lingering problem I found there needs attention. But for now I'd
rather keep this fix as minimally invasive as it gets.
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -430,13 +430,6 @@ xfs_reflink_fill_cow_hole(
> > if (error)
> > return error;
> >
> > - /*
> > - * Allocation succeeded but the requested range was not even partially
> > - * satisfied? Bail out!
> > - */
> > - if (nimaps == 0)
> > - return -ENOSPC;
>
> Hmm. xfs_find_trim_cow_extent returns with @found==false if a delalloc
> reservation appeared in the cow fork while we weren't holding the ILOCK.
> So shouldn't this xfs_bmapi_write call also handle ENOSR?
Probably. Incremental patch, though.
> > - /*
> > - * Allocation succeeded but the requested range was not even
> > - * partially satisfied? Bail out!
> > - */
> > - if (nimaps == 0)
> > - return -ENOSPC;
>
> This xfs_bmapi_write call converts delalloc reservations to unwritten
> mappings, which means that should catch ENOSR and just run around the
> loop again, right?
Probably..
prev parent reply other threads:[~2024-04-05 17:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-05 5:19 [PATCH] xfs: fix error returns from xfs_bmapi_write Christoph Hellwig
2024-04-05 16:07 ` Darrick J. Wong
2024-04-05 17:00 ` Christoph Hellwig [this message]
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=20240405170042.GA15760@lst.de \
--to=hch@lst.de \
--cc=chandanbabu@kernel.org \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=linux-xfs@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 read-only IMAP folder(s) and NNTP newsgroup(s).