Re: [PATCH 15/17] mkfs: don't treat files as though they are block devices

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 01:02:04PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If the device is actually a file, and "-d file" is not specified,
> mkfs will try to treat it as a block device and get stuff wrong.
> Image files don't necessarily have the same sector sizes as the
> block device or filesystem underlying the image file, nor should we
> be issuing discard ioctls on image files.
> 
> To fix this sanely, only require "-d file" if the device name is
> invalid to trigger creation of the file. Otherwise, use stat() to
> determine if the device is a file or block device and deal with that
> appropriately by setting the "isfile" variables and turning off
> direct IO. Then ensure that we check the "isfile" options before
> doing things that are specific to block devices.
> 
> Other file/blockdev issues fixed:
> 	- use getstr to detect specifying the data device name
> 	  twice.
> 	- check file/size/name parameters before anything else.
> 	- overwrite checks need to be done before the image file is
> 	  opened and potentially truncated.
> 	- blkid_get_topology() should not be called for image files,
> 	  so warn when it is called that way.
> 	- zero_old_xfs_structures() emits a spurious error:
> 		"existing superblock read failed: Success"
> 	  when it is run on a truncated image file. Don't warn if we
> 	  see this problem on an image file.
> 	- Don't issue discards on image files.
> 	- Use fsync() for image files, not BLKFLSBUF in
> 	  platform_flush_device() for Linux.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Jan Ťulák <jtulak@redhat.com>
> ---
>  libxfs/init.c   |   4 ++
>  libxfs/linux.c  |  11 +++-
>  mkfs/xfs_mkfs.c | 164 +++++++++++++++++++++++++++++++++++++++-----------------
>  3 files changed, 129 insertions(+), 50 deletions(-)
> 
> diff --git a/libxfs/init.c b/libxfs/init.c
> index 6f404aa..ed97043 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -246,6 +246,7 @@ libxfs_init(libxfs_init_t *a)
>  	char		rtpath[25];
>  	int		rval = 0;
>  	int		flags;
> +	struct stat st;

Variable alignment.

>  
>  	dpath[0] = logpath[0] = rtpath[0] = '\0';
>  	dname = a->dname;
> @@ -278,6 +279,9 @@ libxfs_init(libxfs_init_t *a)
>  			a->ddev= libxfs_device_open(dname, a->dcreat, flags,
>  						    a->setblksize);
>  			a->dfd = libxfs_device_to_fd(a->ddev);
> +			stat(dname, &st);
> +			a->dsize = st.st_size;
> +			a->dbsize = st.st_blksize;

Error handling?

>  		} else {
>  			if (!check_open(dname, flags, &rawfile, &blockfile))
>  				goto done;
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index 885016a..49d430c 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -125,7 +125,16 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
>  void
>  platform_flush_device(int fd, dev_t device)
>  {
> -	if (major(device) != RAMDISK_MAJOR)
> +	struct stat64	st;
> +	if (major(device) == RAMDISK_MAJOR)
> +		return;
> +
> +	if (fstat64(fd, &st) < 0)
> +		return;
> +
> +	if (S_ISREG(st.st_mode))
> +		fsync(fd);
> +	else
>  		ioctl(fd, BLKFLSBUF, 0);
>  }
>  
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 6bfa73c..ce64230 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -705,7 +705,7 @@ calc_stripe_factors(
>   */
>  static int
>  check_overwrite(
> -	char		*device)
> +	const char	*device)
>  {
>  	const char	*type;
>  	blkid_probe	pr = NULL;
> @@ -722,7 +722,7 @@ check_overwrite(
>  	fd = open(device, O_RDONLY);
>  	if (fd < 0)
>  		goto out;
> -	platform_findsizes(device, fd, &size, &bsz);
> +	platform_findsizes((char *)device, fd, &size, &bsz);
>  	close(fd);
>  
>  	/* nothing to overwrite on a 0-length device */
> @@ -769,7 +769,6 @@ check_overwrite(
>  			"according to blkid\n"), progname, device);
>  	}
>  	ret = 1;
> -
>  out:
>  	if (pr)
>  		blkid_free_probe(pr);
> @@ -795,8 +794,12 @@ static void blkid_get_topology(
>  	struct stat statbuf;
>  
>  	/* can't get topology info from a file */
> -	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
> +	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
> +		fprintf(stderr,
> +	_("%s: Warning: trying to probe topology of a file %s!\n"),
> +			progname, device);
>  		return;
> +	}
>  
>  	pr = blkid_new_probe_from_filename(device);
>  	if (!pr)
> @@ -899,7 +902,7 @@ static void get_topology(
>  #else /* ENABLE_BLKID */
>  static int
>  check_overwrite(
> -	char		*device)
> +	const char	*device)
>  {
>  	char		*type;
>  
> @@ -956,6 +959,75 @@ static void get_topology(
>  #endif /* ENABLE_BLKID */
>  
>  static void
> +check_device_type(
> +	const char	*name,
> +	int		*isfile,
> +	bool		no_size,
> +	bool		no_name,
> +	int		*create,
> +	bool		force_overwrite,
> +	const char	*optname)
> +{
> +	struct stat64 statbuf;
> +
> +	if (*isfile && (no_size || no_name)) {
> +		fprintf(stderr,
> +	_("if -%s file then -%s name and -%s size are required\n"),
> +			optname, optname, optname);
> +		usage();
> +	}
> +
> +	if (stat64(name, &statbuf)) {
> +		if (errno == ENOENT && *isfile) {
> +			if (create)
> +				*create = 1;
> +			return;
> +		}
> +
> +		fprintf(stderr,
> +	_("Error accessing specified device %s: %s\n"),
> +				name, strerror(errno));
> +		usage();
> +		return;
> +	}
> +
> +	if (!force_overwrite && check_overwrite(name)) {
> +		fprintf(stderr,
> +	_("%s: Use the -f option to force overwrite.\n"),
> +			progname);
> +		exit(1);
> +	}
> +
> +	/*
> +	 * We only want to completely truncate and recreate an existing file if
> +	 * we were specifically told it was a file. Set the create flag only in
> +	 * this case to trigger that behaviour.
> +	 */
> +	if (S_ISREG(statbuf.st_mode)) {
> +		if (!*isfile)
> +			*isfile = 1;
> +		else if (create)
> +			*create = 1;
> +		return;
> +	}
> +
> +	if (S_ISBLK(statbuf.st_mode)) {
> +		if (*isfile) {
> +			fprintf(stderr,
> +	_("specified \"-%s file\" on a block device %s\n"),
> +				optname, name);
> +			usage();
> +		}
> +		return;
> +	}
> +
> +	fprintf(stderr,
> +	_("specified device %s not a file or block device\n"),
> +		name);
> +	usage();
> +}
> +
> +static void
>  fixup_log_stripe_unit(
>  	int		lsflag,
>  	int		sunit,
> @@ -1245,9 +1317,18 @@ zero_old_xfs_structures(
>  			strerror(errno));
>  		goto done;
>  	}
> -	if (tmp != new_sb->sb_sectsize) {
> -		fprintf(stderr,
> -	_("warning: could not read existing superblock, skip zeroing\n"));
> +	/*
> +	 * If we are creating an image file, it might be of zero length at this
> +	 * point in time. Hence reading the existing superblock is going to
> +	 * return zero bytes. It's not a failure we need to warn about in this
> +	 * case.
> +	 */
> +	off = pread(xi->dfd, buf, new_sb->sb_sectsize, 0);
> +	if (off != new_sb->sb_sectsize) {
> +		if (!xi->disfile)
> +			fprintf(stderr,
> +	_("error reading existing superblock: %s\n"),
> +				strerror(errno));

This appears to be duplicate code. See the immediately previous pread()
in zero_old_xfs_structures().

>  		goto done;
>  	}
>  	libxfs_sb_from_disk(&sb, buf);
> @@ -1713,8 +1794,6 @@ main(
>  				case D_FILE:
>  					xi.disfile = getnum(value, &dopts,
>  							    D_FILE);
> -					if (xi.disfile && !Nflag)
> -						xi.dcreat = 1;
>  					break;
>  				case D_NAME:
>  					xi.dname = getstr(value, &dopts, D_NAME);
> @@ -1843,8 +1922,6 @@ main(
>  				case L_FILE:
>  					xi.lisfile = getnum(value, &lopts,
>  							    L_FILE);
> -					if (xi.lisfile)
> -						xi.lcreat = 1;
>  					break;
>  				case L_INTERNAL:
>  					loginternal = getnum(value, &lopts,
> @@ -2011,8 +2088,6 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  				case R_FILE:
>  					xi.risfile = getnum(value, &ropts,
>  							    R_FILE);
> -					if (xi.risfile)
> -						xi.rcreat = 1;
>  					break;
>  				case R_NAME:
>  				case R_DEV:
> @@ -2079,13 +2154,7 @@ _("cannot specify both -m crc=1 and -n ftype\n"));
>  		fprintf(stderr, _("extra arguments\n"));
>  		usage();
>  	} else if (argc - optind == 1) {
> -		dfile = xi.volname = argv[optind];
> -		if (xi.dname) {
> -			fprintf(stderr,
> -				_("cannot specify both %s and -d name=%s\n"),
> -				xi.volname, xi.dname);
> -			usage();
> -		}
> +		dfile = xi.volname = getstr(argv[optind], &dopts, D_NAME);

Shouldn't this be part of the previous patch?

Brian

>  	} else
>  		dfile = xi.dname;
>  
> @@ -2118,6 +2187,26 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
>  		lsectorsize = sectorsize;
>  	}
>  
> +	/*
> +	 * Before anything else, verify that we are correctly operating on
> +	 * files or block devices and set the control parameters correctly.
> +	 * Explicitly disable direct IO for image files so we don't error out on
> +	 * sector size mismatches between the new filesystem and the underlying
> +	 * host filesystem.
> +	 */
> +	check_device_type(dfile, &xi.disfile, !dsize, !xi.dname,
> +			  Nflag ? NULL : &xi.dcreat, force_overwrite, "d");
> +	if (!loginternal)
> +		check_device_type(xi.logname, &xi.lisfile, !logsize, !xi.logname,
> +				  Nflag ? NULL : &xi.lcreat,
> +				  force_overwrite, "l");
> +	if (xi.rtname)
> +		check_device_type(xi.rtname, &xi.risfile, !rtsize, !xi.rtname,
> +				  Nflag ? NULL : &xi.rcreat,
> +				  force_overwrite, "r");
> +	if (xi.disfile || xi.lisfile || xi.risfile)
> +		xi.isdirect = 0;
> +
>  	memset(&ft, 0, sizeof(ft));
>  	get_topology(&xi, &ft, force_overwrite);
>  
> @@ -2278,11 +2367,6 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	}
>  
>  
> -	if (xi.disfile && (!dsize || !xi.dname)) {
> -		fprintf(stderr,
> -	_("if -d file then -d name and -d size are required\n"));
> -		usage();
> -	}
>  	if (dsize) {
>  		__uint64_t dbytes;
>  
> @@ -2315,11 +2399,6 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  		usage();
>  	}
>  
> -	if (xi.lisfile && (!logsize || !xi.logname)) {
> -		fprintf(stderr,
> -		_("if -l file then -l name and -l size are required\n"));
> -		usage();
> -	}
>  	if (logsize) {
>  		__uint64_t logbytes;
>  
> @@ -2337,11 +2416,6 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  				(long long)logbytes, blocksize,
>  				(long long)(logblocks << blocklog));
>  	}
> -	if (xi.risfile && (!rtsize || !xi.rtname)) {
> -		fprintf(stderr,
> -		_("if -r file then -r name and -r size are required\n"));
> -		usage();
> -	}
>  	if (rtsize) {
>  		__uint64_t rtbytes;
>  
> @@ -2463,22 +2537,14 @@ _("warning: sparse inodes not supported without CRC support, disabled.\n"));
>  	xi.rtsize &= sector_mask;
>  	xi.logBBsize &= (__uint64_t)-1 << (MAX(lsectorlog, 10) - BBSHIFT);
>  
> -	if (!force_overwrite) {
> -		if (check_overwrite(dfile) ||
> -		    check_overwrite(logfile) ||
> -		    check_overwrite(xi.rtname)) {
> -			fprintf(stderr,
> -			_("%s: Use the -f option to force overwrite.\n"),
> -				progname);
> -			exit(1);
> -		}
> -	}
>  
> +	/* don't do discards on print-only runs or on files */
>  	if (discard && !Nflag) {
> -		discard_blocks(xi.ddev, xi.dsize);
> -		if (xi.rtdev)
> +		if (!xi.disfile)
> +			discard_blocks(xi.ddev, xi.dsize);
> +		if (xi.rtdev && !xi.risfile)
>  			discard_blocks(xi.rtdev, xi.rtsize);
> -		if (xi.logdev && xi.logdev != xi.ddev)
> +		if (xi.logdev && xi.logdev != xi.ddev && !xi.lisfile)
>  			discard_blocks(xi.logdev, xi.logBBsize);
>  	}
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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