[PATCH] mkfs.xfs: always use underlying fs sector size when mkfs'ing a file

[Eric Sandeen]

On 6/18/15 6:03 AM, Jan Tulak wrote:
> 
> 
> ----- Original Message -----
>> From: "Eric Sandeen" <sandeen at redhat.com>
>> To: "xfs-oss" <xfs at oss.sgi.com>
>> Sent: Thursday, June 18, 2015 1:26:33 AM
>> Subject: [PATCH] mkfs.xfs: always use underlying fs sector size when mkfs'ing	a file
>>
>> If we are mkfs'ing a file, and that file is on a 4k sector filesystem,
>> we should make the fs image file with the same sector size, or things
>> may fail when they try to do direct IO in 512 byte chunks (depending
>> on whether it is a 512e or "hard" 4k device).
>>
>> Earlier commits attempted this to some degree:
>>
>> 5a7d59 xfsprogs: try to handle mkfs of a file on 4k sector device
>> 3800a2 mkfs.xfs: don't call blkid_get_topology on existing regular files
>>
>> but inexplicably missed the case where mkfs.xfs with "-d file" was
>> specified.
>>
>> One more try; in get_topology(), try to get the underlying fs sector
>> size in *all* cases where we are mkfs'ing a file, and set the sector size
>> accordingly.
>>
>> Signed-off-by: Eric Sandeen <sandeen at redhat.com>
>> ---
>>
>> (This does it for 512e as well as hard 4k drives, but I think that's
>> probably ok?  If not, perhaps we should go further and attempt to
>> discern logical and physical sectors for the device under the
>> filesystem.  Is it worth it?  Not sure it is.)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index e2a052d..e44c390 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -462,31 +462,34 @@ static void get_topology(
>>  	struct fs_topology	*ft,
>>  	int			force_overwrite)
>>  {
>> -	if (!xi->disfile) {
>> -		char *dfile = xi->volname ? xi->volname : xi->dname;
>> -		struct stat statbuf;
>> +	struct stat statbuf;
>> +	char *dfile = xi->volname ? xi->volname : xi->dname;
>>  
>> -		/*
>> -		 * If our target is a regular file, and xi->disfile isn't
>> -		 * set (i.e. no "-d file" invocation), use platform_findsizes
>> -		 * to try to obtain the underlying filesystem's requirements
>> -		 * for direct IO; we'll set our sector size to that if possible.
>> -		 */
>> -		if (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode)) {
>> -			int fd;
>> -			long long dummy;
>> -
>> -			fd = open(dfile, O_RDONLY);
>> -			if (fd >= 0) {
>> -				platform_findsizes(dfile, fd, &dummy,
>> -						   &ft->lsectorsize);
>> -				close(fd);
>> -			}
>> -		} else {
>> -			blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
>> -					   &ft->lsectorsize, &ft->psectorsize,
>> -					   force_overwrite);
>> +	/*
>> +	 * If our target is a regular file, use platform_findsizes
>> +	 * to try to obtain the underlying filesystem's requirements
>> +	 * for direct IO; we'll set our sector size to that if possible.
>> +	 */
>> +	if (xi->disfile ||
>> +	    (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {
>> +		int fd;
>> +		int flags = O_RDONLY;
>> +		long long dummy;
>> +
>> +		/* with xi->disfile we may not have the file yet! */
>> +		if (xi->disfile)
>> +			flags |= O_CREAT;
>> +
>> +		fd = open(dfile, flags, 0666);
>> +		if (fd >= 0) {
>> +			platform_findsizes(dfile, fd, &dummy, &ft->lsectorsize);
>> +			close (fd);
>>  		}
>> +
>> +	} else {
>> +		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
>> +				   &ft->lsectorsize, &ft->psectorsize,
>> +				   force_overwrite);
>>  	}
>>  
>>  	if (xi->rtname && !xi->risfile) {
>>
> 
> This changes get_topology only for ENABLE_BLKID branch of #ifdef. Is
> that intentional, i.e. we don't expect anyone not using ENABLE_BLKID?
> Because otherwise, if mkfs is compiled without ENABLE_BLKID, then all
> we get is:

Hm, yeah, good point.  I always forget about this.  :(  I can send V2.

And sorry if this overlaps w/ your changes- I got a bug report about
xfstests failing when testing hard 4k devices, and it was due to image
files created on a filesystem on that hard 4k device, and xfsprogs tools
fail when they try to do 512-byte direct IO to the image.

Thanks,
-Eric