Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 25 May 2023 12:00:43 -0400
From:   Mike Snitzer <snitzer@kernel.org>
To:     Dave Chinner <david@fromorbit.com>, Joe Thornber <ejt@redhat.com>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org,
        Theodore Ts'o <tytso@mit.edu>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        "Darrick J. Wong" <djwong@kernel.org>,
        Brian Foster <bfoster@redhat.com>,
        Bart Van Assche <bvanassche@google.com>,
        linux-kernel@vger.kernel.org, Joe Thornber <ejt@redhat.com>,
        Christoph Hellwig <hch@infradead.org>, dm-devel@redhat.com,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Sarthak Kukreti <sarthakkukreti@chromium.org>,
        linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        Jason Wang <jasowang@redhat.com>,
        Alasdair Kergon <agk@redhat.com>
Subject: Re: [PATCH v7 0/5] Introduce provisioning primitives
Message-ID: <ZG+GKwFC7M3FfAO5@redhat.com>
References: <20230518223326.18744-1-sarthakkukreti@chromium.org>
 <ZGb2Xi6O3i2pLam8@infradead.org>
 <ZGeKm+jcBxzkMXQs@redhat.com>
 <ZGgBQhsbU9b0RiT1@dread.disaster.area>
 <ZGu0LaQfREvOQO4h@redhat.com>
 <ZGzIJlCE2pcqQRFJ@bfoster>
 <ZGzbGg35SqMrWfpr@redhat.com>
 <ZG1dAtHmbQ53aOhA@dread.disaster.area>
 <ZG5taYoXDRymo/e9@redhat.com>
 <ZG9JD+4Zu36lnm4F@dread.disaster.area>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZG9JD+4Zu36lnm4F@dread.disaster.area>
Precedence: bulk

On Thu, May 25 2023 at  7:39P -0400,
Dave Chinner <david@fromorbit.com> wrote:

> On Wed, May 24, 2023 at 04:02:49PM -0400, Mike Snitzer wrote:
> > On Tue, May 23 2023 at  8:40P -0400,
> > Dave Chinner <david@fromorbit.com> wrote:
> > 
> > > On Tue, May 23, 2023 at 11:26:18AM -0400, Mike Snitzer wrote:
> > > > On Tue, May 23 2023 at 10:05P -0400, Brian Foster <bfoster@redhat.com> wrote:
> > > > > On Mon, May 22, 2023 at 02:27:57PM -0400, Mike Snitzer wrote:
> > > > > ... since I also happen to think there is a potentially interesting
> > > > > development path to make this sort of reserve pool configurable in terms
> > > > > of size and active/inactive state, which would allow the fs to use an
> > > > > emergency pool scheme for managing metadata provisioning and not have to
> > > > > track and provision individual metadata buffers at all (dealing with
> > > > > user data is much easier to provision explicitly). So the space
> > > > > inefficiency thing is potentially just a tradeoff for simplicity, and
> > > > > filesystems that want more granularity for better behavior could achieve
> > > > > that with more work. Filesystems that don't would be free to rely on the
> > > > > simple/basic mechanism provided by dm-thin and still have basic -ENOSPC
> > > > > protection with very minimal changes.
> > > > > 
> > > > > That's getting too far into the weeds on the future bits, though. This
> > > > > is essentially 99% a dm-thin approach, so I'm mainly curious if there's
> > > > > sufficient interest in this sort of "reserve mode" approach to try and
> > > > > clean it up further and have dm guys look at it, or if you guys see any
> > > > > obvious issues in what it does that makes it potentially problematic, or
> > > > > if you would just prefer to go down the path described above...
> > > > 
> > > > The model that Dave detailed, which builds on REQ_PROVISION and is
> > > > sticky (by provisioning same blocks for snapshot) seems more useful to
> > > > me because it is quite precise.  That said, it doesn't account for
> > > > hard requirements that _all_ blocks will always succeed.
> > > 
> > > Hmmm. Maybe I'm misunderstanding the "reserve pool" context here,
> > > but I don't think we'd ever need a hard guarantee from the block
> > > device that every write bio issued from the filesystem will succeed
> > > without ENOSPC.
> > > 
> > > If the block device can provide a guarantee that a provisioned LBA
> > > range is always writable, then everything else is a filesystem level
> > > optimisation problem and we don't have to involve the block device
> > > in any way. All we need is a flag we can ready out of the bdev at
> > > mount time to determine if the filesystem should be operating with
> > > LBA provisioning enabled...
> > > 
> > > e.g. If we need to "pre-provision" a chunk of the LBA space for
> > > filesystem metadata, we can do that ahead of time and track the
> > > pre-provisioned range(s) in the filesystem itself.
> > > 
> > > In XFS, That could be as simple as having small chunks of each AG
> > > reserved to metadata (e.g. start with the first 100MB) and limiting
> > > all metadata allocation free space searches to that specific block
> > > range. When we run low on that space, we pre-provision another 100MB
> > > chunk and then allocate all metadata out of that new range. If we
> > > start getting ENOSPC to pre-provisioning, then we reduce the size of
> > > the regions and log low space warnings to userspace. If we can't
> > > pre-provision any space at all and we've completely run out, we
> > > simply declare ENOSPC for all incoming operations that require
> > > metadata allocation until pre-provisioning succeeds again.
> > 
> > This is basically saying the same thing but:
> > 
> > It could be that the LBA space is fragmented and so falling back to
> > the smallest region size (that matches the thinp block size) would be
> > the last resort?  Then if/when thinp cannot even service allocating a
> > new free thin block, dm-thinp will transition to out-of-data-space
> > mode.
> 
> Yes, something of that sort, though we'd probably give up if we
> can't get at least megabyte scale reservations - a single
> modification in XFS can modify many structures and require
> allocation of a lot of new metadata, so the fileystem cut-off would
> for metadata provisioning failure would be much larger than the
> dm-thinp region size....
> 
> > > This is built entirely on the premise that once proactive backing
> > > device provisioning fails, the backing device is at ENOSPC and we
> > > have to wait for that situation to go away before allowing new data
> > > to be ingested. Hence the block device really doesn't need to know
> > > anything about what the filesystem is doing and vice versa - The
> > > block dev just says "yes" or "no" and the filesystem handles
> > > everything else.
> > 
> > Yes.
> > 
> > > It's worth noting that XFS already has a coarse-grained
> > > implementation of preferred regions for metadata storage. It will
> > > currently not use those metadata-preferred regions for user data
> > > unless all the remaining user data space is full.  Hence I'm pretty
> > > sure that a pre-provisioning enhancment like this can be done
> > > entirely in-memory without requiring any new on-disk state to be
> > > added.
> > > 
> > > Sure, if we crash and remount, then we might chose a different LBA
> > > region for pre-provisioning. But that's not really a huge deal as we
> > > could also run an internal background post-mount fstrim operation to
> > > remove any unused pre-provisioning that was left over from when the
> > > system went down.
> > 
> > This would be the FITRIM with extension you mention below? Which is a
> > filesystem interface detail?
> 
> No. We might reuse some of the internal infrastructure we use to
> implement FITRIM, but that's about it. It's just something kinda
> like FITRIM but with different constraints determined by the
> filesystem rather than the user...
> 
> As it is, I'm not sure we'd even need it - a preiodic userspace
> FITRIM would acheive the same result, so leaked provisioned spaces
> would get cleaned up eventually without the filesystem having to do
> anything specific...
> 
> > So dm-thinp would _not_ need to have new
> > state that tracks "provisioned but unused" block?
> 
> No idea - that's your domain. :)
> 
> dm-snapshot, for certain, will need to track provisioned regions
> because it has to guarantee that overwrites to provisioned space in
> the origin device will always succeed. Hence it needs to know how
> much space breaking sharing in provisioned regions after a snapshot
> has been taken with be required...

dm-thinp offers its own much more scalable snapshot support (doesn't
use old dm-snapshot N-way copyout target).

dm-snapshot isn't going to be modified to support this level of
hardening (dm-snapshot is basically in "maintenance only" now).

But I understand your meaning: what you said is 100% applicable to
dm-thinp's snapshot implementation and needs to be accounted for in
thinp's metadata (inherent 'provisioned' flag).

> > Nor would the block
> > layer need an extra discard flag for a new class of "provisioned"
> > blocks.
> 
> Right, I don't see that the discard operations need to care whether
> the underlying storage is provisioned. dm-thinp and dm-snapshot can
> treat REQ_OP_DISCARD as "this range is not longer in use" and do
> whatever they want with them. 
> 
> > If XFS tracked this "provisioned but unused" state, dm-thinp could
> > just discard the block like its told.  Would be nice to avoid dm-thinp
> > needing to track "provisioned but unused".
> >
> > That said, dm-thinp does still need to know if a block was provisioned
> > (given our previous designed discussion, to allow proper guarantees
> > from this interface at snapshot time) so that XFS and other
> > filesystems don't need to re-provision areas they already
> > pre-provisioned.
> 
> Right.
> 
> I've simply assumed that dm-thinp would need to track entire
> provisioned regions - used or unused - so it knows which writes to
> empty or shared regions have a reservation to allow allocation to
> succeed when the backing pool is otherwise empty.....
> 
> > However, it may be that if thinp did track "provisioned but unused"
> > it'd be useful to allow snapshots to share provisioned blocks that
> > were never used.  Meaning, we could then avoid "breaking sharing" at
> > snapshot-time for "provisioned but unused" blocks.  But allowing this
> > "optimization" undercuts the gaurantee that XFS needs for thinp
> > storage that allows snapshots... SO, I think I answered my own
> > question: thinp doesnt need to track "provisioned but unused" blocks
> > but we must always ensure snapshots inherit provisoned blocks ;)
> 
> Sounds like a potential optimisation, but I haven't thought through
> a potential snapshot device implementation that far to comment
> sanely. I stopped once I got to the point where accounting tricks
> count be used to guarantee space is available for breaking sharing
> of used provisioned space after a snapshot was taken....
> 
> > > Further, managing shared pool exhaustion doesn't require a
> > > reservation pool in the backing device and for the filesystems to
> > > request space from it. Filesystems already have their own reserve
> > > pools via pre-provisioning. If we want the filesystems to be able to
> > > release that space back to the shared pool (e.g. because the shared
> > > backing pool is critically short on space) then all we need is an
> > > extension to FITRIM to tell the filesystem to also release internal
> > > pre-provisioned reserves.
> > 
> > So by default FITRIM will _not_ discard provisioned blocks.  Only if
> > a flag is used will it result in discarding provisioned blocks.
> 
> No. FITRIM results in discard of any unused free space in the
> filesystem that matches the criteria set by the user. We don't care
> if free space was once provisioned used space - we'll issue a
> discard for the range regardless. The "special" FITRIM extension I
> mentioned is to get filesystem metadata provisioning released;
> that's completely separate to user data provisioning through
> fallocate() which FITRIM will always discard if it has been freed...
> 
> IOWs, normal behaviour will be that a FITRIM ends up discarding a
> mix of unprovisioned and provisioned space. Nobody will be able to
> predict what mix the device is going to get at any point in time.
> Also, if we turn on online discard, the block device is going to get
> a constant stream of discard operations that will also be a mix of
> provisioned and unprovisioned space that is not longer in use by the
> filesystem. 
> 
> I suspect that you need to stop trying to double guess what
> operations the filesystem will use provisioning for, what it will
> send discards for and when it will send discards for them.. Just
> assume the device will receive a constant stream of both
> REQ_PROVISION and REQ_OP_DISCARD (for both provisioned and
> unprovisioned regions) operations whenver the filesystem is active
> on a thinp device.....

Yeah, I was getting tripped up in the weeds a bit.  It's pretty
straight-forward (and like I said at the start of our subthread here:
this follow-on work, to inherit provisioned flag, can build on this
REQ_PROVISION patchset).

All said, I've now gotten this sub-thread on Joe Thornber's radar and
we've started discussing. We'll be discussing with more focus
tomorrow.

Mike