Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.7\))
Subject: Re: [PATCH v2] mmp: do not use O_DIRECT when working with regular
 file
From:   Alexey Lyashkov <alexey.lyashkov@gmail.com>
In-Reply-To: <BF8274AF-A9C6-40F4-8B99-FEBA82878C36@dilger.ca>
Date:   Fri, 19 Feb 2021 13:08:17 +0300
Cc:     Lukas Czerner <lczerner@redhat.com>, linux-ext4@vger.kernel.org,
        Eric Sandeen <sandeen@redhat.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <99A17D19-8764-4027-8B1E-E7ADBE5E2CEE@gmail.com>
References: <20210212093719.162065-1-lczerner@redhat.com>
 <20210218095146.265302-1-lczerner@redhat.com>
 <BF8274AF-A9C6-40F4-8B99-FEBA82878C36@dilger.ca>
To:     Andreas Dilger <adilger@dilger.ca>,
        Artem Blagodarenko <artem.blagodarenko@gmail.com>
Precedence: bulk

Andreas,

What about to disable a O_DIRECT global on any block devices in the =
e2fsprogs library as this don=E2=80=99t work on 4k disk drives at all ?
Instead of fixing an O_DIRECT access with patches sends early.


Alex

> 19 =D1=84=D0=B5=D0=B2=D1=80. 2021 =D0=B3., =D0=B2 1:20, Andreas Dilger =
<adilger@dilger.ca> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
>=20
> On Feb 18, 2021, at 2:51 AM, Lukas Czerner <lczerner@redhat.com> =
wrote:
>>=20
>> Currently the mmp block is read using O_DIRECT to avoid any caching =
that
>> may be done by the VM. However when working with regular files this
>> creates alignment issues when the device of the host file system has
>> sector size larger than the blocksize of the file system in the file
>> we're working with.
>>=20
>> This can be reproduced with t_mmp_fail test when run on the device =
with
>> 4k sector size because the mke2fs fails when trying to read the mmp
>> block.
>>=20
>> Fix it by disabling O_DIRECT when working with regular files. I don't
>> think there is any risk of doing so since the file system layer, =
unlike
>> shared block device, should guarantee cache consistency.
>>=20
>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>=20
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
>=20
>> ---
>> v2: Fix comment - it avoids problems when the sector size is larger =
not
>>   smaller than blocksize
>>=20
>> lib/ext2fs/mmp.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>=20
>> diff --git a/lib/ext2fs/mmp.c b/lib/ext2fs/mmp.c
>> index c21ae272..cca2873b 100644
>> --- a/lib/ext2fs/mmp.c
>> +++ b/lib/ext2fs/mmp.c
>> @@ -57,21 +57,21 @@ errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t =
mmp_blk, void *buf)
>> 	 * regardless of how the io_manager is doing reads, to avoid =
caching of
>> 	 * the MMP block by the io_manager or the VM.  It needs to be =
fresh. */
>> 	if (fs->mmp_fd <=3D 0) {
>> +		struct stat st;
>> 		int flags =3D O_RDWR | O_DIRECT;
>>=20
>> -retry:
>> +		/*
>> +		 * There is no reason for using O_DIRECT if we're =
working with
>> +		 * regular file. Disabling it also avoids problems with
>> +		 * alignment when the device of the host file system has =
sector
>> +		 * size larger than blocksize of the fs we're working =
with.
>> +		 */
>> +		if (stat(fs->device_name, &st) =3D=3D 0 &&
>> +		    S_ISREG(st.st_mode))
>> +			flags &=3D ~O_DIRECT;
>> +
>> 		fs->mmp_fd =3D open(fs->device_name, flags);
>> 		if (fs->mmp_fd < 0) {
>> -			struct stat st;
>> -
>> -			/* Avoid O_DIRECT for filesystem image files if =
open
>> -			 * fails, since it breaks when running on tmpfs. =
*/
>> -			if (errno =3D=3D EINVAL && (flags & O_DIRECT) &&
>> -			    stat(fs->device_name, &st) =3D=3D 0 &&
>> -			    S_ISREG(st.st_mode)) {
>> -				flags &=3D ~O_DIRECT;
>> -				goto retry;
>> -			}
>> 			retval =3D EXT2_ET_MMP_OPEN_DIRECT;
>> 			goto out;
>> 		}
>> --
>> 2.26.2
>>=20
>=20
>=20
> Cheers, Andreas
>=20
>=20
>=20
>=20
>=20