mkfs: a possible bad

[Jan Tulak]

----- Original Message -----
> From: "Eric Sandeen" <sandeen at sandeen.net>
> To: "Jan Tulak" <jtulak at redhat.com>, xfs at oss.sgi.com
> 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: http://oss.sgi.com/archives/xfs/2013-11/msg00847.html
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. 


Cheers,
Jan

-- 
Jan Tulak
jtulak at redhat.com