Re: [PATCH 15/19] mkfs: don't treat files as though they are block devices

Jan Tulak
2016-04-08T15:57:20Z

[-- Attachment #1.1: Type: text/plain, Size: 4005 bytes --]

On Fri, Apr 8, 2016 at 5:50 PM, Eric Sandeen <sandeen@sandeen.net> wrote:

> On 4/8/16 9:58 AM, Jan Tulak wrote:
> > Ok, platform_findsizes already explicitly handled regular files, and
> tries to
> > find out via an xfs ioctl what the minimum DIO size is, and uses
> that for
> > the sector size for the filesystem in the iamge.
> >
> >
> > Now you stat & get the blocksize, and use that instead, but it's
> likely
> > to be different:
> >
> > i.e. before:
> >
> > # mkfs/mkfs.xfs -f fsfile
> > meta-data=fsfile isize=512 agcount=4,
> agsize=65536 blks
> > = sectsz=512 attr=2, projid32bit=1
> >
> > after:
> >
> > # mkfs/mkfs.xfs -f fsfile
> > meta-data=fsfile isize=512 agcount=4,
> agsize=65536 blks
> > = sectsz=4096 attr=2, projid32bit=1
> >
> > and also, now:
> >
> > # mkfs/mkfs.xfs -f -dfile,name=fsfile,size=1g -b size=2048
> > block size 2048 cannot be smaller than logical sector size 4096
> >
> > What prompted you to make this change, was there some other problem
> you
> > needed to fix?
> >
> >
> > ​But DIO is disabled for the files, per the commit message:
> > [...] and turning off
> > direct IO. Then ensure that we check the "isfile" options before
> > doing things that are specific to block devices. Also, as direct IO
> > is disabled for files, use statfs() for getting host FS blocksize,
> > not platform_findsizes().​
>
> But doing DIO to the file during mkfs isn't the issue I'm talking about;
> For example a vm image hosted in a file will have direct IO done to it
> when it is in use, and filesystem block size is not the controlling
> factor there.
>
> With your change, we can no longer make i.e. a 2k fs image hosted on a 4k
> fs. i.e. your change regresses this commit:
>
> commit 98dd72d3b239050138cf9eb9373c83743878a7d2
> Author: Eric Sandeen <sandeen@sandeen.net>
> Date: Fri Dec 18 12:15:25 2015 +1100
>
> mkfs: get sector size from host fs d_miniosz when mkfs'ing file
>
> Now that we allow logical-sector-sized DIOs even if our xfs
> filesystem is set to physical-sector-sized "sectors," we can
> allow the creation of filesystem images with block and sector
> sizes down to the host device's logical sector size, rather
> than the filesystem's sector size.
>
> So in platform_findsizes(), change our query of the filesystem
> to an XFS_IOC_DIOINFO query, and use the returned d_miniosz for
> sector size in the S_IFREG case.
>
> This allows the creation of i.e. a 2k block sized image on
> an xfs filesystem w/ 4k sector size created on a 4k/512
> block device, which would otherwise fail today.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Dave Chinner <david@fromorbit.com>
>
>
> > So we have to use whatever the underlying fs tells us, not what the
> physical device has, right?
>
> Well, tells us about what? XFS can tell us its block size, but also can
> tell us about minimum size and alignment required for direct IO, which is
> more relevant to a filesystem image than the filesystem's block size.
>
> > ​ Rather, I wonder if there is any reason to keep the
> > platform_findsizes part about regular files - it shouldn't get into
> > the branch ever.
>
> In general, having a wrapper which finds sizes of a target, regardless of
> platform, device, or file, makes sense to me rather than having stat calls
> in various other places...
>
> -Eric
>

It seems I misunderstood some things about what ​happens/what should happen
with files. :-)
I will see what happens with the tests, what difference in the results is
with this patch in the original version.

​Cheers,
Jan​

--
Jan Tulak
jtulak@redhat.com / jan@tulak.me

[-- Attachment #1.2: Type: text/html, Size: 5967 bytes --]

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]