On Fri, Sep 13, 2019 at 11:02:37AM +1000, Dave Chinner wrote:
> On Thu, Sep 05, 2019 at 08:35:22PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > spaceman/health.c | 4 +-
> > 7 files changed, 105 insertions(+), 32 deletions(-)
> >
> >
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index cc3cc93a..e8fa39ab 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -724,6 +724,7 @@ fsrfile(
> > xfs_ino_t ino)
> > {
> > struct xfs_fd fsxfd = XFS_FD_INIT_EMPTY;
> > + struct xfs_bulkstat bulkstat;
> > struct xfs_bstat statbuf;
> > jdm_fshandle_t *fshandlep;
> > int fd = -1;
> > @@ -748,12 +749,13 @@ fsrfile(
> > goto out;
> > }
> >
> > - error = xfrog_bulkstat_single(&fsxfd, ino, &statbuf);
> > + error = xfrog_bulkstat_single(&fsxfd, ino, 0, &bulkstat);
> > if (error) {
> > fsrprintf(_("unable to get bstat on %s: %s\n"),
> > fname, strerror(error));
> > goto out;
> > }
> > + xfrog_bulkstat_to_bstat(&fsxfd, &statbuf, &bulkstat);
>
> So this is so none of the rest of fsr needs to be converted to use
> the new structure versions? When will this go away?
>
> > do {
> > - struct xfs_bstat tbstat;
> > + struct xfs_bulkstat tbstat;
> > char name[64];
> > int ret;
> >
> > @@ -983,7 +985,7 @@ fsr_setup_attr_fork(
> > * this to compare against the target and determine what we
> > * need to do.
> > */
> > - ret = xfrog_bulkstat_single(&txfd, tstatbuf.st_ino, &tbstat);
> > + ret = xfrog_bulkstat_single(&txfd, tstatbuf.st_ino, 0, &tbstat);
> > if (ret) {
> > fsrprintf(_("unable to get bstat on temp file: %s\n"),
> > strerror(ret));
>
> Because this looks like we now have a combination of v1 and v5
> structures being used...
>
> >
> > diff --git a/io/swapext.c b/io/swapext.c
> > index 2b4918f8..ca024b93 100644
> > --- a/io/swapext.c
> > +++ b/io/swapext.c
> > @@ -28,6 +28,7 @@ swapext_f(
> > char **argv)
> > {
> > struct xfs_fd fxfd = XFS_FD_INIT(file->fd);
> > + struct xfs_bulkstat bulkstat;
> > int fd;
> > int error;
> > struct xfs_swapext sx;
> > @@ -48,12 +49,13 @@ swapext_f(
> > goto out;
> > }
> >
> > - error = xfrog_bulkstat_single(&fxfd, stat.st_ino, &sx.sx_stat);
> > + error = xfrog_bulkstat_single(&fxfd, stat.st_ino, 0, &bulkstat);
> > if (error) {
> > errno = error;
> > perror("bulkstat");
> > goto out;
> > }
> > + xfrog_bulkstat_to_bstat(&fxfd, &sx.sx_stat, &bulkstat);
>
> and this is required because bstat is part of the swapext ioctl ABI?
Yes. I think a lof of the retained bulkstat weirdness in fsr could go
away if (a) we maintained an open xfs_fd to the filesystem and (b)
wrapped the swapext ioctl... but there's already too much here so I
stopped short of refactoring fsr.
> > sx.sx_version = XFS_SX_VERSION;
> > sx.sx_fdtarget = file->fd;
> > sx.sx_fdtmp = fd;
> > diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
> > index b4468243..2a70824e 100644
> > --- a/libfrog/bulkstat.c
> > +++ b/libfrog/bulkstat.c
> > @@ -20,26 +20,99 @@ xfrog_bulkstat_prep_v1_emulation(
> > return xfd_prepare_geometry(xfd);
> > }
> >
> > +/* Bulkstat a single inode using v5 ioctl. */
> > +static int
> > +xfrog_bulkstat_single5(
> > + struct xfs_fd *xfd,
> > + uint64_t ino,
> > + unsigned int flags,
> > + struct xfs_bulkstat *bulkstat)
> > +{
> > + struct xfs_bulkstat_req *req;
> > + int ret;
> > +
> > + if (flags & ~(XFS_BULK_IREQ_SPECIAL))
> > + return EINVAL;
>
> negative error returns, please.
>
> > @@ -57,8 +57,6 @@ xfs_iterate_inodes_range_check(
> > int error;
> >
> > for (i = 0, bs = bstat; i < XFS_INODES_PER_CHUNK; i++) {
> > - struct xfs_bstat bs1;
> > -
> > if (!(inogrp->xi_allocmask & (1ULL << i)))
> > continue;
> > if (bs->bs_ino == inogrp->xi_startino + i) {
> > @@ -68,13 +66,11 @@ xfs_iterate_inodes_range_check(
> >
> > /* Load the one inode. */
> > error = xfrog_bulkstat_single(&ctx->mnt,
> > - inogrp->xi_startino + i, &bs1);
> > - if (error || bs1.bs_ino != inogrp->xi_startino + i) {
> > + inogrp->xi_startino + i, 0, bs);
> > + if (error || bs->bs_ino != inogrp->xi_startino + i) {
> > memset(bs, 0, sizeof(struct xfs_bulkstat));
> > bs->bs_ino = inogrp->xi_startino + i;
> > bs->bs_blksize = ctx->mnt_sv.f_frsize;
> > - } else {
> > - xfrog_bstat_to_bulkstat(&ctx->mnt, bs, &bs1);
> > }
> > bs++;
> > }
>
> So this immediately tears down the confusing stuff that was set up
> in the previous patch. Perhaps separate out the scrub changes and do
> both bulkstat and bulkstat_single conversions in one patch?
Ok.
--D
> -Dave.
>
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
