xfs
[Top] [All Lists]

Re: [PATCH] mkfs: handle 4k sector devices more cleanly

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] mkfs: handle 4k sector devices more cleanly
From: Eric Sandeen <sandeen@xxxxxxxxxx>
Date: Thu, 10 Dec 2009 17:00:32 -0600
Cc: xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <20091210224037.GA28342@xxxxxxxxxxxxx>
References: <4B1E9A25.50108@xxxxxxxxxx> <20091210224037.GA28342@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.21 (X11/20090320)
Christoph Hellwig wrote:
> On Tue, Dec 08, 2009 at 12:25:41PM -0600, Eric Sandeen wrote:
>> Trying to mkfs a 4k sector device today fails w/o manually specifying
>> sector size:
>>
>> # modprobe scsi_debug sector_size=4096 dev_size_mb=32
>> # mkfs.xfs -f /dev/sdc
>> mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid 
>> argument
>> Warning: the data subvolume sector size 512 is less than the sector size 
>> reported by the device (4096).
>> ... <fail>
>>
>> add sectorsize to the device topology info, and use that if present.
>>
>> Also check that explicitly requested sector sizes are not smaller
>> than the hardware size.  This already fails today, but with the more
>> cryptic "cannot set blocksize" ioctl error above.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>>
>> diff --git a/libxfs/linux.c b/libxfs/linux.c
>> index bc49903..2e07d54 100644
>> --- a/libxfs/linux.c
>> +++ b/libxfs/linux.c
>> @@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, 
>> int blocksize, int fata
>>      if (major(device) != RAMDISK_MAJOR) {
>>              if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
>>                      fprintf(stderr, _("%s: %s - cannot set blocksize "
>> -                                    "on block device %s: %s\n"),
>> +                                    "%d on block device %s: %s\n"),
>>                              progname, fatal ? "error": "warning",
>> -                            path, strerror(errno));
>> +                            blocksize, path, strerror(errno));
> 
> Defintively a more useful error message than before, thanks.
> 
>> -static void blkid_get_topology(const char *device, int *sunit, int *swidth)
>> +static void blkid_get_topology(const char *device, int *sunit, int *swidth, 
>> int *sectorsize)
>>  {
>>      blkid_topology tp;
>>      blkid_probe pr;
>> @@ -348,7 +349,9 @@ static void blkid_get_topology(const char *device, int 
>> *sunit, int *swidth)
>>      val = blkid_topology_get_optimal_io_size(tp) >> 9;
>>      if (val > 1)
>>              *swidth = val;
>> -
>> +    val = blkid_probe_get_sectorsize(pr);
>> +    if (val > 1)
>> +            *sectorsize = val;
> 
> I don't think the val > 1 check here makes any sense.

TBH it was a cut and paste from above, sigh.

I guess I don't know why the other one uses "1" either:

unsigned long blkid_topology_get_optimal_io_size(blkid_topology tp)
{
        return tp ? tp->optimal_io_size : 0;
}

anyway for this one it can be an unconditional assignment I guess.

>> +            blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth, 
>> &ft->sectorsize);
> 
> The lines is growing a bit too long here..

yeah ....

> 
> Also I think you should add the sector size retrival by using the ioctl
> directly for the non-blkid case to make sure the code doesn't have too
> many corner cases.

Ok.  The problem is platform_get_blocksize or whatnot would
want both an fd and a path, and we've not opened anything yet :(

blkid was so handy :)

>> -    if (ft.sectoralign) {
>> -            sectorsize = blocksize;
>> +    /*
>> +     * MD wants sector size set == block size to avoid switching.
>> +     * Otherwise, if not specfied via command, use device sectorsize
>> +     */
>> +    if (ft.sectoralign || !ssflag) {
>> +            if (ft.sectoralign)
>> +                    sectorsize = blocksize;
>> +            else
>> +                    sectorsize = ft.sectorsize;
> 
> The code looks good, but I don't think the comment helps understanding
> what's going on.  And I might get a bit pendantic here, but changing it
> to
> 
>       if (!ssflag || ft.sectoralign)
> 
> might make the intent a bit more clear.

ok. I can drop the comment, doesn't bother me.

> 
>> +    if (sectorsize < ft.sectorsize) {
>> +            fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
>> +                    sectorsize, ft.sectorsize);
>> +            usage();  
>> +    }
> 
> Looks good.
> 
>>      if (lsectorsize < XFS_MIN_SECTORSIZE ||
>>          lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
>>              fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
>> @@ -1749,10 +1764,10 @@ main(
>>      calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
>>                              &dsunit, &dswidth, &lsunit);
>>  
>> -    if (slflag || ssflag)
>> +    if (slflag || ssflag || ft.setorsize)
> 
> There's a c missing here, this wouldn't even compile :)

boooo for last-minute edits, sorry.

> It will also be always true for blkid builds which is at least a bit
> confusing.  If you also updated the libdisk case as suggested above we
> could also get rid of the xi.setblksize = 1 special case totally and
> always pass the proper block size to libxfs_init and libxfs_device_open
> (and make libxfs_device_open static in libxfs/init.c while we're at it
> :))

hm all good ideas, though need to think about how to do things
cleanly with platform_find_blah.

We already have:

void
platform_findsizes(char *path, int fd, long long *sz, int *bsz)

which would make it easy, but grumble, we don't have an fd yet!

I guess maybe I could make that function cope with a dummy fd,
and do the open/close itself....

Thanks,
-Eric

<Prev in Thread] Current Thread [Next in Thread>