Re: [PATCH, RFC] mkfs: get sector size from host fs dev when mkfs'ing file

[Dave Chinner <david@fromorbit.com>]

On Thu, Dec 03, 2015 at 05:16:46PM -0600, Eric Sandeen wrote:
> Now that we allow logical-sector-sized DIOs even if our xfs
> filesystem is set to physical-sector-sized "sectors," we can
> allow the creation of filesystem images with block and sector
> sizes down to the host device's logical sector size, rather
> than the filesystem's sector size.
> 
> So in platform_findsizes(), change our query of the filesystem
> to a query of the device, and use that for sector size in the
> S_IFREG case.
> 
> This allows the creation of i.e. a 2k block sized image on
> an xfs filesystem w/ 4k sector size created on a 4k/512
> block device, which would otherwise fail today.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> This feels like about my 5th stab at this; it seems like
> the correct thing to do but by now my brain is full.
> Seem sane?

I'm not sure I like the idea of encoding blkid functionality
into libxfs, especially as blkid support is optional:

commit 6635d6ab510c58996f5ed3f212148db5e3c299ba
Author: Jan Tulak <jtulak@redhat.com>
Date:   Wed Oct 14 10:57:39 2015 +1100

    build: make libblkid usage optional


> Still needs a run through xfstests but wanted to see if this
> was obviously bonkers or not...?
> 
> (I don't know why copy/Makefile needs a $(LIBBLKID), either...)

Because libxfs now has a dependency on libblkid, and libxfs is a
static library so it doesn't resolve undefined extern objects as the
library is built. That is only done when linking binaries....

> index 885016a..3a8dc12 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -24,6 +24,7 @@
>  #include <sys/mount.h>
>  #include <sys/ioctl.h>
>  #include <sys/sysinfo.h>
> +#include <blkid/blkid.h>
>  
>  #include "libxfs_priv.h"
>  #include "xfs_fs.h"
> @@ -142,18 +143,29 @@ platform_findsizes(char *path, int fd, long long *sz, int *bsz)
>  			progname, path, strerror(errno));
>  		exit(1);
>  	}
> +
>  	if ((st.st_mode & S_IFMT) == S_IFREG) {
> -		struct xfs_fsop_geom_v1 geom = { 0 };
> +		int	fd;
> +		char	*hostfs_dev_path;	/* path to host fs device */
>  
>  		*sz = (long long)(st.st_size >> 9);
> -		if (ioctl(fd, XFS_IOC_FSGEOMETRY_V1, &geom) < 0) {
> -			/*
> -			 * fall back to BBSIZE; mkfs might fail if there's a
> -			 * size mismatch between the image & the host fs...
> -			 */
> -			*bsz = BBSIZE;
> -		} else
> -			*bsz = geom.sectsize;
> +
> +		/*
> +		 * Default to BBSIZE; mkfs might fail if there's a
> +		 * size mismatch between the image & the host fs...
> +		 */
> +		*bsz = BBSIZE;
> +
> +		/* Get logical sector size of host device */
> +		hostfs_dev_path = blkid_devno_to_devname(st.st_dev);
> +		if (hostfs_dev_path) {
> +			fd = open(hostfs_dev_path, O_RDONLY);
> +			if (fd >= 0)
> +				if (ioctl(fd, BLKSSZGET, bsz) < 0)
> +					*bsz = BBSIZE;
> +			close(fd);
> +			free(hostfs_dev_path);
> +		}

So the current behaviour is to use the underlying filesystem
sector size, but we might have a 4k fs sector on a 512 physical
sector device, and you want to detect this, right? The logical
sector size is exposed by the filesystem in the XFS_IOC_DIOINFO
information. i.e:

        case XFS_IOC_DIOINFO: {
                struct dioattr  da;
                xfs_buftarg_t   *target =
                        XFS_IS_REALTIME_INODE(ip) ?
                        mp->m_rtdev_targp : mp->m_ddev_targp;

                da.d_mem =  da.d_miniosz = target->bt_logical_sectorsize;
                da.d_maxiosz = INT_MAX & ~(da.d_miniosz - 1);

                if (copy_to_user(arg, &da, sizeof(da)))
                        return -EFAULT;
                return 0;
        }

Isn't this exactly what XFS_IOC_DIOINFO is for?

(Oh, and "old kernels don't support...." simply means the distros
will have a kernel patch to backport, too....)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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