----- 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: 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@xxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
