From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Allison Collins <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/9] xfs: wire up new v5 bulkstat ioctls
Date: Fri, 7 Jun 2019 09:10:55 -0700 [thread overview]
Message-ID: <20190607161055.GC1688126@magnolia> (raw)
In-Reply-To: <a57f8117-2a6e-ad62-f374-20dccef458b3@oracle.com>
On Thu, Jun 06, 2019 at 03:37:40PM -0700, Allison Collins wrote:
> On 6/6/19 2:10 PM, Darrick J. Wong wrote:
> > On Wed, Jun 05, 2019 at 03:30:21PM -0700, Allison Collins wrote:
> > > On 5/29/19 3:28 PM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > >
> > > > Wire up the new v5 BULKSTAT ioctl and rename the old one to V1.
> > > >
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > > fs/xfs/libxfs/xfs_fs.h | 24 +++++++++++
> > > > fs/xfs/xfs_ioctl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > fs/xfs/xfs_ioctl32.c | 1
> > > > fs/xfs/xfs_ondisk.h | 1
> > > > 4 files changed, 129 insertions(+), 1 deletion(-)
> > > >
> > > >
> > > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > > index 8b8fe78511fb..960f3542e207 100644
> > > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > > @@ -435,7 +435,6 @@ struct xfs_fsop_bulkreq {
> > > > __s32 __user *ocount; /* output count pointer */
> > > > };
> > > > -
> > > > /*
> > > > * Structures returned from xfs_inumbers routine (XFS_IOC_FSINUMBERS).
> > > > */
> > > > @@ -457,6 +456,28 @@ struct xfs_inumbers {
> > > > #define XFS_INUMBERS_VERSION_V1 (1)
> > > > #define XFS_INUMBERS_VERSION_V5 (5)
> > > > +/* Header for bulk inode requests. */
> > > > +struct xfs_bulk_ireq {
> > > > + uint64_t ino; /* I/O: start with this inode */
> > > > + uint32_t flags; /* I/O: operation flags */
> > > > + uint32_t icount; /* I: count of entries in buffer */
> > > > + uint32_t ocount; /* O: count of entries filled out */
> > > > + uint32_t reserved32; /* must be zero */
> > > > + uint64_t reserved[5]; /* must be zero */
> > > > +};
> > > > +
> > > > +#define XFS_BULK_IREQ_FLAGS_ALL (0)
> > > > +
> > > > +/*
> > > > + * ioctl structures for v5 bulkstat and inumbers requests
> > > > + */
> > > > +struct xfs_bulkstat_req {
> > > > + struct xfs_bulk_ireq hdr;
> > > > + struct xfs_bulkstat bulkstat[];
> > > > +};
> > > > +#define XFS_BULKSTAT_REQ_SIZE(nr) (sizeof(struct xfs_bulkstat_req) + \
> > > > + (nr) * sizeof(struct xfs_bulkstat))
> > > > +
> > > > /*
> > > > * Error injection.
> > > > */
> > > > @@ -758,6 +779,7 @@ struct xfs_scrub_metadata {
> > > > #define XFS_IOC_FSGEOMETRY_V4 _IOR ('X', 124, struct xfs_fsop_geom_v4)
> > > > #define XFS_IOC_GOINGDOWN _IOR ('X', 125, uint32_t)
> > > > #define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom)
> > > > +#define XFS_IOC_BULKSTAT _IOR ('X', 127, struct xfs_bulkstat_req)
> > > > /* XFS_IOC_GETFSUUID ---------- deprecated 140 */
> > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > > index e43ad688e683..f6724c75ba97 100644
> > > > --- a/fs/xfs/xfs_ioctl.c
> > > > +++ b/fs/xfs/xfs_ioctl.c
> > > > @@ -822,6 +822,107 @@ xfs_ioc_fsbulkstat(
> > > > return 0;
> > > > }
> > > > +/* Return 0 on success or positive error */
> > > > +static int
> > > > +xfs_bulkstat_fmt(
> > > > + struct xfs_ibulk *breq,
> > > > + const struct xfs_bulkstat *bstat)
> > > > +{
> > > > + if (copy_to_user(breq->ubuffer, bstat, sizeof(struct xfs_bulkstat)))
> > > > + return -EFAULT;
> > > > + return xfs_ibulk_advance(breq, sizeof(struct xfs_bulkstat));
> > > > +}
> > > > +
> > > > +/*
> > > > + * Check the incoming bulk request @hdr from userspace and initialize the
> > > > + * internal @breq bulk request appropriately. Returns 0 if the bulk request
> > > > + * should proceed; 1 if there's nothing to do; or the usual negative error
> > > > + * code.
> > > > + */
> > > > +static int
> > > > +xfs_bulk_ireq_setup(
> > > > + struct xfs_mount *mp,
> > > > + struct xfs_bulk_ireq *hdr,
> > > > + struct xfs_ibulk *breq,
> > > > + void __user *ubuffer)
> > > > +{
> > > > + if (hdr->icount == 0 ||
> > > > + (hdr->flags & ~XFS_BULK_IREQ_FLAGS_ALL) ||
> > > > + hdr->reserved32 ||
> > > > + memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
> > > > + return -EINVAL;
> > > > +
> > > > + if (XFS_INO_TO_AGNO(mp, hdr->ino) >= mp->m_sb.sb_agcount)
> > > > + goto no_results;
> > > > +
> > > > + breq->ubuffer = ubuffer;
> > > > + breq->icount = hdr->icount;
> > > > + breq->startino = hdr->ino;
> > > > + return 0;
> > > > +no_results:
> > > > + hdr->ocount = 0;
> > > > + return 1;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Update the userspace bulk request @hdr to reflect the end state of the
> > > > + * internal bulk request @breq. If @error is negative then we return just
> > > > + * that; otherwise (@error is 0 or 1) we copy the state so that userspace
> > > > + * can discover what happened.
> > > > + */
> > > > +static int
> > > > +xfs_bulk_ireq_teardown(
> > > > + struct xfs_bulk_ireq *hdr,
> > > > + struct xfs_ibulk *breq,
> > > > + int error)
> > > > +{
> > > > + if (error < 0)
> > > > + return error;
> > > Hmm, passing in error just to bail on error seemed a little out of scope to
> > > me. Is there a reason we're doing it here? Instead of after the preceding
> > > call made in the caller? Referenced below.....
> > >
> > > > +
> > > > + hdr->ino = breq->startino;
> > > > + hdr->ocount = breq->ocount;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* Handle the v5 bulkstat ioctl. */
> > > > +STATIC int
> > > > +xfs_ioc_bulkstat(
> > > > + struct xfs_mount *mp,
> > > > + unsigned int cmd,
> > > > + struct xfs_bulkstat_req __user *arg)
> > > > +{
> > > > + struct xfs_bulk_ireq hdr;
> > > > + struct xfs_ibulk breq = {
> > > > + .mp = mp,
> > > > + };
> > > > + int error;
> > > > +
> > > > + if (!capable(CAP_SYS_ADMIN))
> > > > + return -EPERM;
> > > > +
> > > > + if (XFS_FORCED_SHUTDOWN(mp))
> > > > + return -EIO;
> > > > +
> > > > + if (copy_from_user(&hdr, &arg->hdr, sizeof(hdr)))
> > > > + return -EFAULT;
> > > > +
> > > > + error = xfs_bulk_ireq_setup(mp, &hdr, &breq, arg->bulkstat);
> > > > + if (error < 0)
> > > > + return error;
> > > > +
> > > > + if (!error)
> > > > + error = xfs_bulkstat(&breq, xfs_bulkstat_fmt);
> > > > +
> > > Right here. How about
> > >
> > > if (error < 0)
> > > return error;
> > >
> > > It seems functionally equivalent. If error < 0, teardown will
> > > bounce it back anyway and then the error check below will toss it back up.
> > > Is that what you meant?
> >
> > Yeah, I could do that too. :)
> >
> > TBH I only threw it in as a helper function because xfs_bulk_ireq_setup
> > seemed to need a counterpart; it's such a short function that I could
> > just opencode it... Thoughts?
> >
> > --D
>
> Sure, I'd be fine with open coding it too. I think I made a similar comment
> in patch 6 since these helper functions are so small. I think if you just
> add a comment or two about updating the header that would be fine.
I could even keep the teardown helper and just adjust the calling style:
error = xfs_bulk_ireq_setup(...);
if (error == 1)
goto teardown;
else if (error < 0)
goto out;
error = xfs_bulkstat(...);
if (error)
goto out;
teardown:
error = 0;
xfs_bulk_ireq_teardown(...);
if (copy_to_user(...)))
return -EFAULT;
out:
return error;
Yeah, maybe I'll do that, it's looks more like our normal practice.
--D
> Allison
>
> >
> > > Allison
> > >
> > > > + error = xfs_bulk_ireq_teardown(&hdr, &breq, error);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + if (copy_to_user(&arg->hdr, &hdr, sizeof(hdr)))
> > > > + return -EFAULT;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > STATIC int
> > > > xfs_ioc_fsgeometry(
> > > > struct xfs_mount *mp,
> > > > @@ -1986,6 +2087,9 @@ xfs_file_ioctl(
> > > > case XFS_IOC_FSINUMBERS:
> > > > return xfs_ioc_fsbulkstat(mp, cmd, arg);
> > > > + case XFS_IOC_BULKSTAT:
> > > > + return xfs_ioc_bulkstat(mp, cmd, arg);
> > > > +
> > > > case XFS_IOC_FSGEOMETRY_V1:
> > > > return xfs_ioc_fsgeometry(mp, arg, 3);
> > > > case XFS_IOC_FSGEOMETRY_V4:
> > > > diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> > > > index bfe71747776b..84c342be4536 100644
> > > > --- a/fs/xfs/xfs_ioctl32.c
> > > > +++ b/fs/xfs/xfs_ioctl32.c
> > > > @@ -576,6 +576,7 @@ xfs_file_compat_ioctl(
> > > > case XFS_IOC_ERROR_CLEARALL:
> > > > case FS_IOC_GETFSMAP:
> > > > case XFS_IOC_SCRUB_METADATA:
> > > > + case XFS_IOC_BULKSTAT:
> > > > return xfs_file_ioctl(filp, cmd, p);
> > > > #if !defined(BROKEN_X86_ALIGNMENT) || defined(CONFIG_X86_X32)
> > > > /*
> > > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > > > index d8f941b4d51c..954484c6eb96 100644
> > > > --- a/fs/xfs/xfs_ondisk.h
> > > > +++ b/fs/xfs/xfs_ondisk.h
> > > > @@ -149,6 +149,7 @@ xfs_check_ondisk_structs(void)
> > > > XFS_CHECK_STRUCT_SIZE(struct xfs_bulkstat, 192);
> > > > XFS_CHECK_STRUCT_SIZE(struct xfs_inumbers, 24);
> > > > + XFS_CHECK_STRUCT_SIZE(struct xfs_bulkstat_req, 64);
> > > > }
> > > > #endif /* __XFS_ONDISK_H */
> > > >
next prev parent reply other threads:[~2019-06-07 16:11 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-29 22:27 [PATCH 0/9] xfs: introduce new BULKSTAT and INUMBERS ioctls Darrick J. Wong
2019-05-29 22:27 ` [PATCH 1/9] xfs: remove various bulk request typedef usage Darrick J. Wong
2019-06-05 22:29 ` Allison Collins
2019-05-29 22:27 ` [PATCH 2/9] xfs: rename bulkstat functions Darrick J. Wong
2019-06-05 22:29 ` Allison Collins
2019-05-29 22:27 ` [PATCH 3/9] xfs: introduce new v5 bulkstat structure Darrick J. Wong
2019-06-05 22:29 ` Allison Collins
2019-06-06 20:17 ` Darrick J. Wong
2019-06-06 22:41 ` Allison Collins
2019-05-29 22:27 ` [PATCH 4/9] xfs: introduce v5 inode group structure Darrick J. Wong
2019-06-05 22:30 ` Allison Collins
2019-05-29 22:28 ` [PATCH 5/9] xfs: wire up new v5 bulkstat ioctls Darrick J. Wong
2019-06-05 22:30 ` Allison Collins
2019-06-06 21:10 ` Darrick J. Wong
2019-06-06 22:37 ` Allison Collins
2019-06-07 16:10 ` Darrick J. Wong [this message]
2019-05-29 22:28 ` [PATCH 6/9] xfs: wire up the new v5 bulkstat_single ioctl Darrick J. Wong
2019-06-05 22:30 ` Allison Collins
2019-05-29 22:28 ` [PATCH 7/9] xfs: wire up the v5 INUMBERS ioctl Darrick J. Wong
2019-06-05 22:30 ` Allison Collins
2019-05-29 22:28 ` [PATCH 8/9] xfs: specify AG in bulk req Darrick J. Wong
2019-06-05 22:30 ` Allison Collins
2019-05-29 22:28 ` [PATCH 9/9] xfs: allow bulkstat_single of special inodes Darrick J. Wong
2019-06-05 22:31 ` Allison Collins
2019-06-06 21:15 ` Darrick J. Wong
-- strict thread matches above, loose matches on Subject: below --
2019-06-12 6:49 [PATCH v5 0/9] xfs: introduce new BULKSTAT and INUMBERS ioctls Darrick J. Wong
2019-06-12 6:49 ` [PATCH 5/9] xfs: wire up new v5 bulkstat ioctls Darrick J. Wong
2019-06-26 20:45 [PATCH v6 0/9] xfs: introduce new BULKSTAT and INUMBERS ioctls Darrick J. Wong
2019-06-26 20:46 ` [PATCH 5/9] xfs: wire up new v5 bulkstat ioctls Darrick J. Wong
2019-07-03 13:24 ` 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=20190607161055.GC1688126@magnolia \
--to=darrick.wong@oracle.com \
--cc=allison.henderson@oracle.com \
--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.