[Top] [All Lists]

Re: mkfs: a possible bad

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: mkfs: a possible bad
From: Jan Tulak <jtulak@xxxxxxxxxx>
Date: Thu, 18 Jun 2015 04:53:09 -0400 (EDT)
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <5581CDF3.5030408@xxxxxxxxxxx>
References: <551099978.15978267.1434551318983.JavaMail.zimbra@xxxxxxxxxx> <5581CDF3.5030408@xxxxxxxxxxx>
Thread-index: iN7GHoyl8Zj2BH0FwGLqPYBqLpx1mw==
Thread-topic: mkfs: a possible bad

----- Original Message -----
> From: "Eric Sandeen" <sandeen@xxxxxxxxxxx>
> To: "Jan Tulak" <jtulak@xxxxxxxxxx>, xfs@xxxxxxxxxxx
> Sent: Wednesday, June 17, 2015 9:43:47 PM
> Subject: Re: mkfs: a possible bad
> On 6/17/15 9:28 AM, Jan Tulak wrote:
> > Hi, I'm looking into mkfs/xfs_mkfs.c and I wonder, is "if (xi.dbsize
> > > sectorsize)" correct? It is a check for: Warning: the data
> > subvolume sector size %u is less than the sector size reported by the
> > device (%u).
> > 
> > But psectorsize is assigned to sectorsize, not to xi.dbsize, so the
> > two values seems to be swapped in the condition (and as arguments of
> > the printf too). I think this gone without noticing because usually,
> > when creating a partition, the two values are the same. So even if
> > the condition is wrong, nothing happens. And when -bsize=X is passed,
> > then it is catched earlier and nothing happens again.
> > 
> > Only when I apply a patch that changes how mkfs acts when it gets a
> > file instead of a block device, I start to see the warning, although
> > physical sector size is 512 and block size is set to 4096. The
> > numbers are swapped in the warning too...
> > 
> > I tried to run ./check -g quick and it seems that the change breaks
> > nothing. 
> > 
> > Cheers, Jan
> Hohum, xfs_mkfs.c is such spaghetti-code.  :)  Let me think through
> this as well:
> Ok, we init sectorsize to XFS_MIN_SECTORSIZE at the top of main().
> If we have cmdline args to specify sector size, we reset it there.
> If not specified, we set it to the physical sector size advertised
> by the device.
> And xi.dbsize is set in platform_findsizes, for a device it comes
> from BLKSSZGET, which gives us the logical sector size of the device.
> So this test:
>         if (xi.dbsize > sectorsize) {
>                 fprintf(stderr, _(
> "Warning: the data subvolume sector size %u is less than the sector size \n\
> reported by the device (%u).\n"),
>                         sectorsize, xi.dbsize);
>         }
> is testing whether the logical sector size is greater than the physical
> sector size - which would indeed be a problem (logical is <= physical)

You write "logical is <= physical", but the text of the warning seems to be 
telling "it should be logical >= physical, but isn't." 
I initially thought it should be reversed, but now I'm not sure. Maybe the 
warning could use better words to avoid this confusing?
In which case, I have to go back to bug hunting and see what is causing the 
warning to me...

> however, you are right that this does seem to be caught sooner:
> # modprobe scsi_debug sector_size=4096 dev_size_mb=1024 op_blks=0
> # mkfs.xfs -s size=512 /dev/sdd
> illegal sector size 512; hw sector is 4096
> If we're mkfs'ing a file, then platform_findsizes gives us either
> the sector size of the host filesystem (if it's an xfs filesystem),
> or 512 by default if that fails (other filesystems don't have that
> concept, or that ioctl interface).
> But the same thing applies; if we're mkfsing an image on a 4k sector
> xfs fs, images hosted there should (probably) have 4k sectors as well.
> So, I ... don't think I see a problem with the code as it stands.
> What does your "patch that changes how mkfs acts when it gets a file"
> do?
> -Eric

I'm resurrecting this patch set for cleaning mkfs: 
The link is directly to the patch which changes detection and handling for 
files. I did some changes in it, but as a reference, it still works well. 


Jan Tulak

<Prev in Thread] Current Thread [Next in Thread>