From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 2/2] xfs: simplify the xfs_getbmap interface
Date: Wed, 20 Sep 2017 16:08:31 -0700 [thread overview]
Message-ID: <20170920230831.GD7112@magnolia> (raw)
In-Reply-To: <20170918152630.24592-3-hch@lst.de>
On Mon, Sep 18, 2017 at 08:26:30AM -0700, Christoph Hellwig wrote:
> Instead of passing in a formatter callback allocate the bmap buffer
> in the caller and process the entries there. Additionally replace
> the in-kernel buffer with a new much smaller structure, and unify
> the implementation of the different ioctls in a single function.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> fs/xfs/xfs_bmap_util.c | 38 ++++-----------
> fs/xfs/xfs_bmap_util.h | 10 ++--
> fs/xfs/xfs_ioctl.c | 122 ++++++++++++++++++++++++-------------------------
> 3 files changed, 75 insertions(+), 95 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index a87d05978c92..b540ac65b8b3 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -407,11 +407,11 @@ static int
> xfs_getbmap_report_one(
> struct xfs_inode *ip,
> struct getbmapx *bmv,
> - struct getbmapx *out,
> + struct kgetbmap *out,
> int64_t bmv_end,
> struct xfs_bmbt_irec *got)
> {
> - struct getbmapx *p = out + bmv->bmv_entries;
> + struct kgetbmap *p = out + bmv->bmv_entries;
> bool shared = false, trimmed = false;
> int error;
>
> @@ -458,12 +458,12 @@ static void
> xfs_getbmap_report_hole(
> struct xfs_inode *ip,
> struct getbmapx *bmv,
> - struct getbmapx *out,
> + struct kgetbmap *out,
> int64_t bmv_end,
> xfs_fileoff_t bno,
> xfs_fileoff_t end)
> {
> - struct getbmapx *p = out + bmv->bmv_entries;
> + struct kgetbmap *p = out + bmv->bmv_entries;
>
> if (bmv->bmv_iflags & BMV_IF_NO_HOLES)
> return;
> @@ -511,47 +511,36 @@ xfs_getbmap_next_rec(
> */
> int /* error code */
> xfs_getbmap(
> - xfs_inode_t *ip,
> + struct xfs_inode *ip,
> struct getbmapx *bmv, /* user bmap structure */
> - xfs_bmap_format_t formatter, /* format to user */
> - void *arg) /* formatter arg */
> + struct kgetbmap *out)
> {
> struct xfs_mount *mp = ip->i_mount;
> int iflags = bmv->bmv_iflags;
> - int whichfork, lock, i, error = 0;
> + int whichfork, lock, error = 0;
> int64_t bmv_end, max_len;
> xfs_fileoff_t bno, first_bno;
> struct xfs_ifork *ifp;
> - struct getbmapx *out;
> struct xfs_bmbt_irec got, rec;
> xfs_filblks_t len;
> xfs_extnum_t idx;
>
> + if (bmv->bmv_iflags & ~BMV_IF_VALID)
> + return -EINVAL;
> #ifndef DEBUG
> /* Only allow CoW fork queries if we're debugging. */
> if (iflags & BMV_IF_COWFORK)
> return -EINVAL;
> #endif
> -
> if ((iflags & BMV_IF_ATTRFORK) && (iflags & BMV_IF_COWFORK))
> return -EINVAL;
>
> - if (bmv->bmv_count <= 1)
> - return -EINVAL;
> - if (bmv->bmv_count > ULONG_MAX / sizeof(struct getbmapx))
> - return -ENOMEM;
> -
> if (bmv->bmv_length < -1)
> return -EINVAL;
> -
> bmv->bmv_entries = 0;
> if (bmv->bmv_length == 0)
> return 0;
>
> - out = kmem_zalloc_large(bmv->bmv_count * sizeof(struct getbmapx), 0);
> - if (!out)
> - return -ENOMEM;
> -
> if (iflags & BMV_IF_ATTRFORK)
> whichfork = XFS_ATTR_FORK;
> else if (iflags & BMV_IF_COWFORK)
> @@ -698,15 +687,6 @@ xfs_getbmap(
> xfs_iunlock(ip, lock);
> out_unlock_iolock:
> xfs_iunlock(ip, XFS_IOLOCK_SHARED);
> -
> - for (i = 0; i < bmv->bmv_entries; i++) {
> - /* format results & advance arg */
> - error = formatter(&arg, &out[i]);
> - if (error)
> - break;
> - }
> -
> - kmem_free(out);
> return error;
> }
>
> diff --git a/fs/xfs/xfs_bmap_util.h b/fs/xfs/xfs_bmap_util.h
> index 0eaa81dc49be..6cfe747cb142 100644
> --- a/fs/xfs/xfs_bmap_util.h
> +++ b/fs/xfs/xfs_bmap_util.h
> @@ -34,10 +34,14 @@ int xfs_bmap_eof(struct xfs_inode *ip, xfs_fileoff_t endoff,
> int xfs_bmap_punch_delalloc_range(struct xfs_inode *ip,
> xfs_fileoff_t start_fsb, xfs_fileoff_t length);
>
> -/* bmap to userspace formatter - copy to user & advance pointer */
> -typedef int (*xfs_bmap_format_t)(void **, struct getbmapx *);
> +struct kgetbmap {
> + __s64 bmv_offset; /* file offset of segment in blocks */
> + __s64 bmv_block; /* starting block (64-bit daddr_t) */
> + __s64 bmv_length; /* length of segment, blocks */
> + __s32 bmv_oflags; /* output flags */
> +};
> int xfs_getbmap(struct xfs_inode *ip, struct getbmapx *bmv,
> - xfs_bmap_format_t formatter, void *arg);
> + struct kgetbmap *out);
>
> /* functions in xfs_bmap.c that are only needed by xfs_bmap_util.c */
> int xfs_bmap_extsize_align(struct xfs_mount *mp, struct xfs_bmbt_irec *gotp,
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5049e8ab6e30..8e1ab254aa19 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1539,17 +1539,26 @@ xfs_ioc_setxflags(
> return error;
> }
>
> -STATIC int
> -xfs_getbmap_format(void **ap, struct getbmapx *bmv)
> +static bool
> +xfs_getbmap_format(
> + struct kgetbmap *p,
> + struct getbmapx __user *u,
> + size_t recsize)
> {
> - struct getbmap __user *base = (struct getbmap __user *)*ap;
> -
> - /* copy only getbmap portion (not getbmapx) */
> - if (copy_to_user(base, bmv, sizeof(struct getbmap)))
> - return -EFAULT;
> -
> - *ap += sizeof(struct getbmap);
> - return 0;
> + if (put_user(p->bmv_offset, &u->bmv_offset) ||
> + put_user(p->bmv_block, &u->bmv_block) ||
> + put_user(p->bmv_length, &u->bmv_length) ||
> + put_user(0, &u->bmv_count) ||
> + put_user(0, &u->bmv_entries))
> + return false;
> + if (recsize < sizeof(struct getbmapx))
> + return true;
> + if (put_user(0, &u->bmv_iflags) ||
> + put_user(p->bmv_oflags, &u->bmv_oflags) ||
> + put_user(0, &u->bmv_unused1) ||
> + put_user(0, &u->bmv_unused2))
> + return false;
> + return true;
> }
>
> STATIC int
> @@ -1559,68 +1568,57 @@ xfs_ioc_getbmap(
> void __user *arg)
> {
> struct getbmapx bmx = { 0 };
> - int error;
> + struct kgetbmap *buf;
> + size_t recsize;
> + int error, i;
>
> - /* struct getbmap is a strict subset of struct getbmapx. */
> - if (copy_from_user(&bmx, arg, offsetof(struct getbmapx, bmv_iflags)))
> - return -EFAULT;
> -
> - if (bmx.bmv_count < 2)
> + switch (cmd) {
> + case XFS_IOC_GETBMAPA:
> + bmx.bmv_iflags = BMV_IF_ATTRFORK;
> + /*FALLTHRU*/
> + case XFS_IOC_GETBMAP:
> + if (file->f_mode & FMODE_NOCMTIME)
> + bmx.bmv_iflags |= BMV_IF_NO_DMAPI_READ;
> + /* struct getbmap is a strict subset of struct getbmapx. */
> + recsize = sizeof(struct getbmap);
> + break;
> + case XFS_IOC_GETBMAPX:
> + recsize = sizeof(struct getbmapx);
> + break;
> + default:
> return -EINVAL;
> + }
>
> - bmx.bmv_iflags = (cmd == XFS_IOC_GETBMAPA ? BMV_IF_ATTRFORK : 0);
> - if (file->f_mode & FMODE_NOCMTIME)
> - bmx.bmv_iflags |= BMV_IF_NO_DMAPI_READ;
> -
> - error = xfs_getbmap(XFS_I(file_inode(file)), &bmx, xfs_getbmap_format,
> - (__force struct getbmap *)arg+1);
> - if (error)
> - return error;
> -
> - /* copy back header - only size of getbmap */
> - if (copy_to_user(arg, &bmx, sizeof(struct getbmap)))
> - return -EFAULT;
> - return 0;
> -}
> -
> -STATIC int
> -xfs_getbmapx_format(void **ap, struct getbmapx *bmv)
> -{
> - struct getbmapx __user *base = (struct getbmapx __user *)*ap;
> -
> - if (copy_to_user(base, bmv, sizeof(struct getbmapx)))
> - return -EFAULT;
> -
> - *ap += sizeof(struct getbmapx);
> - return 0;
> -}
> -
> -STATIC int
> -xfs_ioc_getbmapx(
> - struct xfs_inode *ip,
> - void __user *arg)
> -{
> - struct getbmapx bmx;
> - int error;
> -
> - if (copy_from_user(&bmx, arg, sizeof(bmx)))
> + if (copy_from_user(&bmx, arg, recsize))
> return -EFAULT;
>
> if (bmx.bmv_count < 2)
> return -EINVAL;
> + if (bmx.bmv_count > ULONG_MAX / recsize)
> + return -ENOMEM;
>
> - if (bmx.bmv_iflags & (~BMV_IF_VALID))
> - return -EINVAL;
> + buf = kmem_zalloc_large(bmx.bmv_count * sizeof(*buf), 0);
> + if (!buf)
> + return -ENOMEM;
>
> - error = xfs_getbmap(ip, &bmx, xfs_getbmapx_format,
> - (__force struct getbmapx *)arg+1);
> + error = xfs_getbmap(XFS_I(file_inode(file)), &bmx, buf);
> if (error)
> - return error;
> + goto out_free_buf;
>
> - /* copy back header */
> - if (copy_to_user(arg, &bmx, sizeof(struct getbmapx)))
> - return -EFAULT;
> + error = -EFAULT;
> + if (copy_to_user(arg, &bmx, recsize))
> + goto out_free_buf;
> + arg += recsize;
> +
> + for (i = 0; i < bmx.bmv_entries; i++) {
> + if (!xfs_getbmap_format(buf + i, arg, recsize))
> + goto out_free_buf;
> + arg += recsize;
> + }
>
> + error = 0;
> +out_free_buf:
> + kmem_free(buf);
> return 0;
> }
>
> @@ -1877,10 +1875,8 @@ xfs_file_ioctl(
>
> case XFS_IOC_GETBMAP:
> case XFS_IOC_GETBMAPA:
> - return xfs_ioc_getbmap(filp, cmd, arg);
> -
> case XFS_IOC_GETBMAPX:
> - return xfs_ioc_getbmapx(ip, arg);
> + return xfs_ioc_getbmap(filp, cmd, arg);
>
> case FS_IOC_GETFSMAP:
> return xfs_ioc_getfsmap(ip, arg);
> --
> 2.14.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-09-20 23:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-18 15:26 getbmap refactor V2 Christoph Hellwig
2017-09-18 15:26 ` [PATCH 1/2] xfs: rewrite getbmap using the xfs_iext_* helpers Christoph Hellwig
2017-09-20 13:23 ` Brian Foster
2017-09-20 14:41 ` Christoph Hellwig
2017-09-20 17:03 ` Darrick J. Wong
2017-09-20 23:00 ` Darrick J. Wong
2017-09-20 23:08 ` Darrick J. Wong
2017-09-21 13:36 ` Christoph Hellwig
2017-09-21 15:35 ` Darrick J. Wong
2017-09-21 13:35 ` Christoph Hellwig
2017-09-21 15:40 ` Darrick J. Wong
2017-09-18 15:26 ` [PATCH 2/2] xfs: simplify the xfs_getbmap interface Christoph Hellwig
2017-09-20 23:08 ` Darrick J. Wong [this message]
-- strict thread matches above, loose matches on Subject: below --
2017-09-03 15:51 [PATCH 1/2] xfs: rewrite getbmap using the xfs_iext_* helpers Christoph Hellwig
2017-09-03 15:51 ` [PATCH 2/2] xfs: simplify the xfs_getbmap interface Christoph Hellwig
2017-09-11 15:49 ` Brian Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170920230831.GD7112@magnolia \
--to=darrick.wong@oracle.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.