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;
Date:   Fri, 19 Feb 2021 11:57:13 +0100
From:   Lukas Czerner <lczerner@redhat.com>
To:     Alexey Lyashkov <alexey.lyashkov@gmail.com>
Cc:     Andreas Dilger <adilger@dilger.ca>,
        Artem Blagodarenko <artem.blagodarenko@gmail.com>,
        linux-ext4@vger.kernel.org, Eric Sandeen <sandeen@redhat.com>
Subject: Re: [PATCH v2] mmp: do not use O_DIRECT when working with regular
 file
Message-ID: <20210219105713.uu2mywenytfd2e5j@work>
References: <20210212093719.162065-1-lczerner@redhat.com>
 <20210218095146.265302-1-lczerner@redhat.com>
 <BF8274AF-A9C6-40F4-8B99-FEBA82878C36@dilger.ca>
 <99A17D19-8764-4027-8B1E-E7ADBE5E2CEE@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <99A17D19-8764-4027-8B1E-E7ADBE5E2CEE@gmail.com>
Precedence: bulk

On Fri, Feb 19, 2021 at 01:08:17PM +0300, Alexey Lyashkov wrote:
> Andreas,
> 
> What about to disable a O_DIRECT global on any block devices in the e2fsprogs library as this don’t work on 4k disk drives at all ?
> Instead of fixing an O_DIRECT access with patches sends early.

Why would it not work at all ? This is a fix for a specific problem and
I am not currently aware of ony other problems e2fsprogs should have
with 4k sector size drives. Do you have a specific problem in mind ?

Thanks!
-Lukas

> 
> 
> Alex
> 
> > 19 февр. 2021 г., в 1:20, Andreas Dilger <adilger@dilger.ca> написал(а):
> > 
> > On Feb 18, 2021, at 2:51 AM, Lukas Czerner <lczerner@redhat.com> wrote:
> >> 
> >> 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.
> >> 
> >> 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.
> >> 
> >> 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.
> >> 
> >> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> >> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> > 
> > Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> > 
> >> ---
> >> v2: Fix comment - it avoids problems when the sector size is larger not
> >>   smaller than blocksize
> >> 
> >> lib/ext2fs/mmp.c | 22 +++++++++++-----------
> >> 1 file changed, 11 insertions(+), 11 deletions(-)
> >> 
> >> 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 <= 0) {
> >> +		struct stat st;
> >> 		int flags = O_RDWR | O_DIRECT;
> >> 
> >> -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) == 0 &&
> >> +		    S_ISREG(st.st_mode))
> >> +			flags &= ~O_DIRECT;
> >> +
> >> 		fs->mmp_fd = 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 == EINVAL && (flags & O_DIRECT) &&
> >> -			    stat(fs->device_name, &st) == 0 &&
> >> -			    S_ISREG(st.st_mode)) {
> >> -				flags &= ~O_DIRECT;
> >> -				goto retry;
> >> -			}
> >> 			retval = EXT2_ET_MMP_OPEN_DIRECT;
> >> 			goto out;
> >> 		}
> >> --
> >> 2.26.2
> >> 
> > 
> > 
> > Cheers, Andreas
> > 
> > 
> > 
> > 
> > 
>