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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
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;
> > }
> >
>
