Re: [PATCH 09/10] iomap: add a IOMAP_DIO_NOALLOC flag

From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@lst.de>,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	avi@scylladb.com
Subject: Re: [PATCH 09/10] iomap: add a IOMAP_DIO_NOALLOC flag
Date: Wed, 13 Jan 2021 10:32:15 -0500	[thread overview]
Message-ID: <20210113153215.GA1284163@bfoster> (raw)
In-Reply-To: <20210112232923.GD331610@dread.disaster.area>

On Wed, Jan 13, 2021 at 10:29:23AM +1100, Dave Chinner wrote:
> On Tue, Jan 12, 2021 at 05:26:15PM +0100, Christoph Hellwig wrote:
> > Add a flag to request that the iomap instances do not allocate blocks
> > by translating it to another new IOMAP_NOALLOC flag.
> 
> Except "no allocation" that is not what XFS needs for concurrent
> sub-block DIO.
> 
> We are trying to avoid external sub-block IO outside the range of
> the user data IO (COW, sub-block zeroing, etc) so that we don't
> trash adjacent sub-block IO in flight. This means we can't do
> sub-block zeroing and that then means we can't map unwritten extents
> or allocate new extents for the sub-block IO.  It also means the IO
> range cannot span EOF because that triggers unconditional sub-block
> zeroing in iomap_dio_rw_actor().
> 
> And because we may have to map multiple extents to fully span an IO
> range, we have to guarantee that subsequent extents for the IO are
> also written otherwise we have a partial write abort case. Hence we
> have single extent limitations as well.
> 
> So "no allocation" really doesn't describe what we want this flag to
> at all.
> 
> If we're going to use a flag for this specific functionality, let's
> call it what it is: IOMAP_DIO_UNALIGNED/IOMAP_UNALIGNED and do two
> things with it.
> 
> 	1. Make unaligned IO a formal part of the iomap_dio_rw()
> 	behaviour so it can do the common checks to for things that
> 	need exclusive serialisation for unaligned IO (i.e. avoid IO
> 	spanning EOF, abort if there are cached pages over the
> 	range, etc).
> 
> 	2. require the filesystem mapping callback do only allow
> 	unaligned IO into ranges that are contiguous and don't
> 	require mapping state changes or sub-block zeroing to be
> 	performed during the sub-block IO.
> 
> 

Something I hadn't thought about before is whether applications might
depend on current unaligned dio serialization for coherency and thus
break if the kernel suddenly allows concurrent unaligned dio to pass
through. Should this be something that is explicitly requested by
userspace?

That aside, I agree that the DIO_UNALIGNED approach seems a bit more
clear than NOALLOC, but TBH the more I look at this the more Christoph's
first approach seems cleanest to me. It is a bit unfortunate to
duplicate the mapping lookups and have the extra ILOCK cycle, but the
lock is shared and only taken when I/O is unaligned. I don't really see
why that is a show stopper yet it's acceptable to fall back to exclusive
dio if the target range happens to be discontiguous (but otherwise
mapped/written).

So I dunno... to me, I would start with that approach and then as the
implementation soaks, perhaps see if we can find a way to optimize away
the extra cycle and lookup. In the meantime, performance should still be
improved significantly and the behavior fairly predictable. Anyways, I
suspect Dave disagrees so that's just my .02. ;) I'll let you guys find
some common ground and make a pass at whatever falls out...

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 