Re: [PATCH] mmp: do not use O_DIRECT when working with regular file

[Lukas Czerner <lczerner@redhat.com>]

On Tue, Feb 16, 2021 at 03:24:00PM -0600, Eric Sandeen wrote:
> On 2/12/21 3:37 AM, Lukas Czerner wrote:
> > Currently the mmp block is read using O_DIRECT to avoid any caching tha
> > 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 smaller 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 file. 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>
> > ---
> >  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..1ac22194 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 smaller than blocksize of the fs we're working with.
> 
> I think the problem is when the host filesystem that contains the image is on
> a device with a logical sector size which is /larger/ than the image filesystem's
> block size, right? Not smaller?

Yeah, it is supposed to be *larger*, of course. If it is smaller, then
there is no problem. Thanks for pointing this out I'll change the
comment and the description.

> 
> Because then you might not be able to do an image-filesystem-block-aligned direct
> IO on it, if it's sub-logical-block-size for the host filesystem/device, and lands
> within the larger host sector at an offset?
> 
> otherwise, this seems at least as reasonable to me as the previous tmpfs work
> around, so other than the question about the comment,
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks!
-Lukas

> 
> 
> > +		 */
> > +		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;
> >  		}
> > 
>