On Wed, Apr 19, 2023 at 4:21 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > On Wed, Apr 19, 2023 at 10:26:02AM -0700, Darrick J. Wong wrote: > > On Wed, Apr 19, 2023 at 12:17:34PM -0400, Mike Snitzer wrote: > > > (And obviously needs fixing independent of this patchset) > > > > > > Shouldn't mkfs first check that the underlying storage supports > > > REQ_OP_PROVISION by verifying > > > /sys/block/<dev>/queue/provision_max_bytes exists and is not 0? > > > (Just saying, we need to add new features more defensively.. you just > > > made the case based on this scenario's implications alone) > > > > Not for fallocate -- for regular files, there's no way to check if the > > filesystem actually supports the operation requested other than to try > > it and see if it succeeds. We probably should've defined a DRY_RUN flag > > for that purpose back when it was introduced. > > That ignores the fact that fallocate() was never intended to > guarantee it will work in all contexts - it's an advisory interface > at it's most basic level. If the call succeeds, then great, it does > what is says on the box, but if it fails then it should have no > visible effect on user data at all and the application still needs > to perform whatever modification it needed done in some other way. > > IOWs, calling it one a block device without first checking if the > block device supports that fallocate operation is exactly how it is > supposed to be used. If the kernel can't handle this gracefully > without corrupting data, then that's a kernel bug and not an > application problem. > > > For fallocate calls to block devices, yes, the program can check the > > queue limits in sysfs if fstat says the supplied path is a block device, > > but I don't know that most programs are that thorough. The fallocate(1) > > CLI program does not check. > > Right. fallocate() was intended to just do the right thing without > the application having to jump thrown an unknown number of hoops to > determine if fallocate() can be used or not in the context it is > executing in. The kernel implementation is supposed to abstract all that > context-dependent behaviour away from the application; all the > application has to do is implement the fallocate() fast path and a > single generic "do the right thing the slow way" fallback when the > fallocate() method it called is not supported... > I added a separate commit[1] to fix this so that we only truncate_bdev_range() iff we are in a supported de-allocate mode call. Subsequently, the REQ_OP_PROVISION patch is a bit simpler when rebased on top. [1] https://www.spinics.net/lists/kernel/msg4765688.html Best Sarthak > -Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
