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..
