Date:   Mon, 22 Aug 2022 21:22:11 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Dave Chinner <david@...morbit.com>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-f2fs-devel@...ts.sourceforge.net,
        xfs <linux-xfs@...r.kernel.org>, linux-api@...r.kernel.org,
        linux-fscrypt@...r.kernel.org,
        linux-block <linux-block@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Keith Busch <kbusch@...nel.org>
Subject: Re: [PATCH v4 6/9] f2fs: don't allow DIO reads but not DIO writes

On Aug 19, 2022, at 5:09 PM, Eric Biggers <ebiggers@...nel.org> wrote:
> 
> On Tue, Aug 16, 2022 at 10:42:29AM -0600, Andreas Dilger wrote:
>> 
>> IMHO, this whole discussion is putting the cart before the horse.
>> Changing existing (and useful) IO behavior to accommodate an API that
>> nobody has ever used, and is unlikely to even be widely used, doesn't
>> make sense to me.  Most applications won't check or care about the new
>> DIO size fields, since they've lived this long without statx() returning
>> this info, and will just pick a "large enough" size (4KB, 1MB, whatever)
>> that gives them the performance they need.  They *WILL* care if the app
>> is suddenly unable to read data from a file in ways that have worked for
>> a long time.
>> 
>> Even if apps are modified to check these new DIO size fields, and then
>> try to DIO write to a file in f2fs that doesn't allow it, then f2fs will
>> return an error, which is what it would have done without the statx()
>> changes, so no harm done AFAICS.
>> 
>> Even with a more-complex DIO status return that handles a "direction"
>> field (which IMHO is needlessly complex), there is always the potential
>> for a TOCTOU race where a file changes between checking and access, so
>> the userspace code would need to handle this.
> 
> I'm having trouble making sense of your argument here; you seem to be saying
> that STATX_DIOALIGN isn't useful, so it doesn't matter if we design it
> correctly?  That line of reasoning is concerning, as it's certainly intended
> to be useful, and if it's not useful there's no point in adding it.
> 
> Are there any specific concerns that you have, besides TOCTOU races and the
> lack of support for read-only DIO?

My main concern is disabling useful functionality that exists today to appease
the new DIO size API.  Whether STATX_DIOALIGN will become widely used by
applications or not is hard to say at this point.

If there were separate STATX_DIOREAD and STATX_DIOWRITE flags in the returned
data, and the alignment is provided as it is today, that would be enough IMHO
to address the original use case without significant complexity.

> I don't think that TOCTOU races are a real concern here.  Generally DIO
> constraints would only change if the application doing DIO intentionally does
> something to the file, or if there are changes that involve the filesystem
> being taken offline, e.g. the filesystem being mounted with significantly
> different options or being moved to a different block device.  And, well,
> everything else in stat()/statx() is subject to TOCTOU as well, but is still
> used...

I was thinking of background filesystem operations like compression, LVM
migration to new storage with a different sector size, etc. that may change
the DIO characteristics of the file even while it is open.  Not that I think
this will happen frequently, but it is possible, and applications shouldn't
explode if the DIO parameters change and they get an error.

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)

Powered by blists - more mailing lists