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

[Jan Tulak <jtulak@xxxxxxxxxx>]

On Fri, Apr 8, 2016 at 5:50 PM, Eric Sandeen <sandeen@xxxxxxxxxxx> 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="" Â Â Â Â Â Â Â Âisize=512Â Â agcount=4, agsize=65536 blks
>Â Â Â Â Â Â Â =Â Â Â Â Â Â Â Â Â Â Â Âsectsz=512Â Âattr=2, projid32bit=1
>
>Â Â Âafter:
>
>Â Â Â# mkfs/mkfs.xfs -f fsfile
>Â Â Âmeta-data="" Â Â Â Â Â Â Â Â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@xxxxxxxxxxx>
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@xxxxxxxxxx>
  Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
  Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>


> 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@xxxxxxxxxxÂ/ jan@xxxxxxxx