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
|