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;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <21923F1E-1C54-44FB-AF7C-4CD8B4B35433@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_8E3621BA-8A02-4654-8DC0-856F43CB7C0C";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v4 6/9] f2fs: don't allow DIO reads but not DIO writes
Date:   Mon, 22 Aug 2022 21:22:11 -0600
In-Reply-To: <YwAYPFxW7VV4M9D1@sol.localdomain>
Cc:     Dave Chinner <david@fromorbit.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Ext4 Developers List <linux-ext4@vger.kernel.org>,
        linux-f2fs-devel@lists.sourceforge.net,
        xfs <linux-xfs@vger.kernel.org>, linux-api@vger.kernel.org,
        linux-fscrypt@vger.kernel.org,
        linux-block <linux-block@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Keith Busch <kbusch@kernel.org>
To:     Eric Biggers <ebiggers@kernel.org>
References: <20220722071228.146690-1-ebiggers@kernel.org>
 <20220722071228.146690-7-ebiggers@kernel.org> <YtyoF89iOg8gs7hj@google.com>
 <Yt7dCcG0ns85QqJe@sol.localdomain> <YuXyKh8Zvr56rR4R@google.com>
 <YvrrEcw4E+rpDLwM@sol.localdomain>
 <20220816090312.GU3600936@dread.disaster.area>
 <D1CDACE3-EC7E-43E4-8F49-EEA2B6E71A41@dilger.ca>
 <YwAYPFxW7VV4M9D1@sol.localdomain>
Precedence: bulk


--Apple-Mail=_8E3621BA-8A02-4654-8DC0-856F43CB7C0C
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Aug 19, 2022, at 5:09 PM, Eric Biggers <ebiggers@kernel.org> wrote:
>=20
> On Tue, Aug 16, 2022 at 10:42:29AM -0600, Andreas Dilger wrote:
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> 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.
>=20
> 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.
>=20
> 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


--Apple-Mail=_8E3621BA-8A02-4654-8DC0-856F43CB7C0C
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmMER+MACgkQcqXauRfM
H+D5gBAAjf5qhCdnFlyRgIkdcPs1zELiCu4+Jw0PH5rkavcu3tVPwEBTCbHxXVMN
6ZsFra7ARjxxj58mUSEtN9NDHwpiQreGlgqDoP+uYYb3EwiFZstja2fl0mTAytE5
lMb8zBxkfCh8xexTnnVk7ULsOrTO+/xYTUbmRIqQvK6PTicVuO6H8FtzYFHjilnn
/RZ8yRCndDsFmRpZCApwy26cPcP3UyrtUqz8rKk1iy5oBIC1ALvx8baYjY/Wi7Fs
vMq1ExdKsjPPVQT5dmj1ISPRXpxTGezKhqvxNvbR/IMwKW2PzEMZzClRQnYKuWe5
GBu6zW0mWe5l+Bg1amzve/WteBX/5i6rrkknoD58zBL8SL4LDh+uQGaM2q7EzegR
lEc1/IpOtR1WrfmP+tOqEIfdwa4jMikNm4NKHYoQA2OQcxEzmYgtBHeeVbGOFCEq
PRKVZbxxF2bVFWuBVbsSbAar9o6G6Ucm11LBDHkKEpn8RTqN/UeXBrcslIk9NczK
2qtnn0jbek1OgtMEpGaajobjeqBqQUIC6/cUkU0S7Ga1LsV7D8eH5TV9EF3rWwLe
0MJ2IWgAmbMDp7gYNVZYrsJJMrqcjphRO33u2JvwFisboKU1DD4qtfnG1vvF8u1n
RHpWdRBE/l0VC15dWox4zVKfVgH52rkeKE8ZVkTj83NDSIidz94=
=WmyJ
-----END PGP SIGNATURE-----

--Apple-Mail=_8E3621BA-8A02-4654-8DC0-856F43CB7C0C--