On Fri, Jun 19, 2015 at 01:02:04PM +0200, Jan Ťulák wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Jan Ťulák <jtulak@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
