mkfs: a possible bad

mkfs: a possible bad

[Jan Tulak]

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

-- 
Jan Tulak
jtulak@redhat.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: mkfs: a possible bad

[Eric Sandeen]

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)

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: mkfs: a possible bad

[Jan Tulak]

----- Original Message -----
> From: "Eric Sandeen" <sandeen@sandeen.net>
> To: "Jan Tulak" <jtulak@redhat.com>, xfs@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@redhat.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: mkfs: a possible bad

[Eric Sandeen]

On 6/18/15 3:53 AM, Jan Tulak wrote:
> 
> 
> ----- Original Message -----
>> From: "Eric Sandeen" <sandeen@sandeen.net>
>> To: "Jan Tulak" <jtulak@redhat.com>, xfs@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...

Sorry, I wasn't very clear.  The test itself is checking if the device's
logical sector size (the smallest possible IO for the device) is 
larger than the filesystem's sector size, which is indeed a problem.

The error message printed does match that, although it states it in
the opposite order.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs