Re: mkfs: a possible bad

From: Eric Sandeen <sandeen@sandeen.net>
To: Jan Tulak <jtulak@redhat.com>
Cc: xfs@oss.sgi.com
Subject: Re: mkfs: a possible bad
Date: Thu, 18 Jun 2015 09:55:21 -0500	[thread overview]
Message-ID: <5582DBD9.7040608@sandeen.net> (raw)
In-Reply-To: <60223436.16408979.1434617589713.JavaMail.zimbra@redhat.com>

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