Re: [PATCH 04/11] xfs: rework the truncate race handling in the writeback path

[Christoph Hellwig <hch@lst.de>]

On Tue, Dec 18, 2018 at 03:03:45PM -0800, Darrick J. Wong wrote:
> > +eof:
> > +	/*
> > +	 * If we raced with truncate there might be no data left at this offset.
> > +	 * In that case we need to return a hole so that the writeback code
> > +	 * skips writeback for the rest of the file.
> > +	 */
> > +	wpc->imap.br_startoff = offset_fsb;
> > +	wpc->imap.br_blockcount = end_fsb - offset_fsb;
> > +	wpc->imap.br_startblock = HOLESTARTBLOCK;
> > +	wpc->imap.br_state = XFS_EXT_NORM;
> > +	return 0;
> 
> This function has become rather spaghetti-like.  Any way we can clean
> this up reasonably?

What is spaghetti about a small number of gotos that we jump to at
the end of the function?  Compared to the previous mess we had this
actually is a significant cleanup.

> > -		nimaps = 0;
> > -		while (nimaps == 0) {
> 
> This removal of the nimaps == 0 loop bothers me: why is doing so safe?
> 
> I see that we can return from xfs_bmapi_write with nimaps == 0 if
> something is trying to punch or truncate the range that we're writing
> back, but it also seems to me that bmapi_write can return zero mappings
> because xfs_bmapi_allocate() didn't find any blocks.  I /think/ that's
> impossible because we're converting delalloc reservations and so we
> should never run out of space, right?
> 
> Anyway, when _write_allocate gets zero mappings, it'll return -EAGAIN to
> xfs_map_blocks, which will retry once to cover the case of racing with
> cow -> data fork remapping but otherwise it won't bother?  And that's
> why it's fine that only to loop once?
> 
> Am I reasoning this correctly?

Yes, exactly.  The only thing the loop did was to make sure we hit
the truncate race handling code another time on a failure return
from xfs_bmapi_write.