mkfs: a possible bad

Hi, I'm looking into mkfs/xfs_mkfs.c and I wonder, is "if (xi.dbsize

Quoted 1 line

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

From: "Eric Sandeen" <[email protected]>
To: "Jan Tulak" <[email protected]>, [email protected]
Sent: Wednesday, June 17, 2015 9:43:47 PM
Subject: Re: mkfs: a possible bad

Quoted 17 lines · Jan Tulak

Hi, I'm looking into mkfs/xfs_mkfs.c and I wonder, is "if (xi.dbsize

Quoted 1 line

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...

Quoted 4 lines

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)

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

----- Original Message -----

Quoted 50 lines

From: "Eric Sandeen" <[email protected]>
To: "Jan Tulak" <[email protected]>, [email protected]
Sent: Wednesday, June 17, 2015 9:43:47 PM
Subject: Re: mkfs: a possible bad

Quoted 17 lines · Jan Tulak

Hi, I'm looking into mkfs/xfs_mkfs.c and I wonder, is "if (xi.dbsize

Quoted 1 line

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...

Quoted 4 lines

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);
        }

Quoted 2 lines

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...