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

[Jan Tulak]

On Fri, Apr 8, 2016 at 5:50 PM, Eric Sandeen <sandeen at 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 at 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 at redhat.com>
>     Reviewed-by: Dave Chinner <dchinner at redhat.com>
>     Signed-off-by: Dave Chinner <david at 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 at redhat.com / jan at tulak.me
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.sgi.com/pipermail/xfs/attachments/20160408/cb568ee2/attachment.html>