* [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
@ 2020-10-05 16:37 Darrick J. Wong
2020-10-09 11:18 ` Brian Foster
2020-10-12 19:29 ` Eric Sandeen
0 siblings, 2 replies; 6+ messages in thread
From: Darrick J. Wong @ 2020-10-05 16:37 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
From: Darrick J. Wong <darrick.wong@oracle.com>
The function scrub_scan_estimate_blocks naïvely uses the statvfs counts
to estimate the size and free blocks on the data volume. Unfortunately,
it fails to account for the fact that statvfs can return the size and
free counts for the realtime volume if the root directory has the
rtinherit flag set, which leads to phase 7 reporting totally absurd
quantities.
Eric pointed out a further problem with statvfs, which is that the file
counts are clamped to the current user's project quota inode limits.
Therefore, we must not use statvfs for querying the filesystem summary
counts.
The XFS_IOC_FSCOUNTS ioctl returns all the data we need, so use that
instead.
Fixes: 604dd3345f35 ("xfs_scrub: filesystem counter collection functions")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: drop statvfs entirely
---
scrub/fscounters.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index f9d64f8c008f..e9901fcdf6df 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -130,38 +130,19 @@ scrub_scan_estimate_blocks(
unsigned long long *f_free)
{
struct xfs_fsop_counts fc;
- struct xfs_fsop_resblks rb;
- struct statvfs sfs;
int error;
- /* Grab the fstatvfs counters, since it has to report accurately. */
- error = fstatvfs(ctx->mnt.fd, &sfs);
- if (error)
- return errno;
-
/* Fetch the filesystem counters. */
error = ioctl(ctx->mnt.fd, XFS_IOC_FSCOUNTS, &fc);
if (error)
return errno;
- /*
- * XFS reserves some blocks to prevent hard ENOSPC, so add those
- * blocks back to the free data counts.
- */
- error = ioctl(ctx->mnt.fd, XFS_IOC_GET_RESBLKS, &rb);
- if (error)
- return errno;
-
- sfs.f_bfree += rb.resblks_avail;
-
- *d_blocks = sfs.f_blocks;
- if (ctx->mnt.fsgeom.logstart > 0)
- *d_blocks += ctx->mnt.fsgeom.logblocks;
- *d_bfree = sfs.f_bfree;
+ *d_blocks = ctx->mnt.fsgeom.datablocks;
+ *d_bfree = fc.freedata;
*r_blocks = ctx->mnt.fsgeom.rtblocks;
*r_bfree = fc.freertx;
- *f_files = sfs.f_files;
- *f_free = sfs.f_ffree;
+ *f_files = fc.allocino;
+ *f_free = fc.freeino;
return 0;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
2020-10-05 16:37 [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts Darrick J. Wong
@ 2020-10-09 11:18 ` Brian Foster
2020-10-09 11:32 ` Brian Foster
2020-10-12 19:29 ` Eric Sandeen
1 sibling, 1 reply; 6+ messages in thread
From: Brian Foster @ 2020-10-09 11:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, xfs
On Mon, Oct 05, 2020 at 09:37:37AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The function scrub_scan_estimate_blocks naïvely uses the statvfs counts
> to estimate the size and free blocks on the data volume. Unfortunately,
> it fails to account for the fact that statvfs can return the size and
> free counts for the realtime volume if the root directory has the
> rtinherit flag set, which leads to phase 7 reporting totally absurd
> quantities.
>
> Eric pointed out a further problem with statvfs, which is that the file
> counts are clamped to the current user's project quota inode limits.
> Therefore, we must not use statvfs for querying the filesystem summary
> counts.
>
> The XFS_IOC_FSCOUNTS ioctl returns all the data we need, so use that
> instead.
>
> Fixes: 604dd3345f35 ("xfs_scrub: filesystem counter collection functions")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: drop statvfs entirely
> ---
This doesn't seem to apply to for-next..?
Brian
> scrub/fscounters.c | 27 ++++-----------------------
> 1 file changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index f9d64f8c008f..e9901fcdf6df 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -130,38 +130,19 @@ scrub_scan_estimate_blocks(
> unsigned long long *f_free)
> {
> struct xfs_fsop_counts fc;
> - struct xfs_fsop_resblks rb;
> - struct statvfs sfs;
> int error;
>
> - /* Grab the fstatvfs counters, since it has to report accurately. */
> - error = fstatvfs(ctx->mnt.fd, &sfs);
> - if (error)
> - return errno;
> -
> /* Fetch the filesystem counters. */
> error = ioctl(ctx->mnt.fd, XFS_IOC_FSCOUNTS, &fc);
> if (error)
> return errno;
>
> - /*
> - * XFS reserves some blocks to prevent hard ENOSPC, so add those
> - * blocks back to the free data counts.
> - */
> - error = ioctl(ctx->mnt.fd, XFS_IOC_GET_RESBLKS, &rb);
> - if (error)
> - return errno;
> -
> - sfs.f_bfree += rb.resblks_avail;
> -
> - *d_blocks = sfs.f_blocks;
> - if (ctx->mnt.fsgeom.logstart > 0)
> - *d_blocks += ctx->mnt.fsgeom.logblocks;
> - *d_bfree = sfs.f_bfree;
> + *d_blocks = ctx->mnt.fsgeom.datablocks;
> + *d_bfree = fc.freedata;
> *r_blocks = ctx->mnt.fsgeom.rtblocks;
> *r_bfree = fc.freertx;
> - *f_files = sfs.f_files;
> - *f_free = sfs.f_ffree;
> + *f_files = fc.allocino;
> + *f_free = fc.freeino;
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
2020-10-09 11:18 ` Brian Foster
@ 2020-10-09 11:32 ` Brian Foster
2020-10-09 15:37 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Brian Foster @ 2020-10-09 11:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Eric Sandeen, xfs
On Fri, Oct 09, 2020 at 07:18:12AM -0400, Brian Foster wrote:
> On Mon, Oct 05, 2020 at 09:37:37AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > The function scrub_scan_estimate_blocks naïvely uses the statvfs counts
> > to estimate the size and free blocks on the data volume. Unfortunately,
> > it fails to account for the fact that statvfs can return the size and
> > free counts for the realtime volume if the root directory has the
> > rtinherit flag set, which leads to phase 7 reporting totally absurd
> > quantities.
> >
> > Eric pointed out a further problem with statvfs, which is that the file
> > counts are clamped to the current user's project quota inode limits.
> > Therefore, we must not use statvfs for querying the filesystem summary
> > counts.
> >
> > The XFS_IOC_FSCOUNTS ioctl returns all the data we need, so use that
> > instead.
> >
> > Fixes: 604dd3345f35 ("xfs_scrub: filesystem counter collection functions")
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: drop statvfs entirely
> > ---
>
> This doesn't seem to apply to for-next..?
>
Oops, never mind. Wrong tree...
> Brian
>
> > scrub/fscounters.c | 27 ++++-----------------------
> > 1 file changed, 4 insertions(+), 23 deletions(-)
> >
> > diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> > index f9d64f8c008f..e9901fcdf6df 100644
> > --- a/scrub/fscounters.c
> > +++ b/scrub/fscounters.c
> > @@ -130,38 +130,19 @@ scrub_scan_estimate_blocks(
> > unsigned long long *f_free)
> > {
> > struct xfs_fsop_counts fc;
> > - struct xfs_fsop_resblks rb;
> > - struct statvfs sfs;
> > int error;
> >
> > - /* Grab the fstatvfs counters, since it has to report accurately. */
> > - error = fstatvfs(ctx->mnt.fd, &sfs);
> > - if (error)
> > - return errno;
> > -
> > /* Fetch the filesystem counters. */
> > error = ioctl(ctx->mnt.fd, XFS_IOC_FSCOUNTS, &fc);
> > if (error)
> > return errno;
> >
> > - /*
> > - * XFS reserves some blocks to prevent hard ENOSPC, so add those
> > - * blocks back to the free data counts.
> > - */
> > - error = ioctl(ctx->mnt.fd, XFS_IOC_GET_RESBLKS, &rb);
> > - if (error)
> > - return errno;
> > -
> > - sfs.f_bfree += rb.resblks_avail;
> > -
> > - *d_blocks = sfs.f_blocks;
> > - if (ctx->mnt.fsgeom.logstart > 0)
> > - *d_blocks += ctx->mnt.fsgeom.logblocks;
> > - *d_bfree = sfs.f_bfree;
> > + *d_blocks = ctx->mnt.fsgeom.datablocks;
> > + *d_bfree = fc.freedata;
> > *r_blocks = ctx->mnt.fsgeom.rtblocks;
> > *r_bfree = fc.freertx;
> > - *f_files = sfs.f_files;
> > - *f_free = sfs.f_ffree;
> > + *f_files = fc.allocino;
> > + *f_free = fc.freeino;
> >
Aren't the free inode counters semantically different between statvfs
and this ioctl? I thought stat had some logic to effectively show free
data blocks as free inodes, whereas the ioctl looks like it just reads
our internal counter (which IIRC is a subset of physically allocated
inode chunks). Do we care about that semantic here either way?
Brian
> > return 0;
> > }
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
2020-10-09 11:32 ` Brian Foster
@ 2020-10-09 15:37 ` Darrick J. Wong
2020-10-15 8:16 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2020-10-09 15:37 UTC (permalink / raw)
To: Brian Foster; +Cc: Eric Sandeen, xfs
On Fri, Oct 09, 2020 at 07:32:25AM -0400, Brian Foster wrote:
> On Fri, Oct 09, 2020 at 07:18:12AM -0400, Brian Foster wrote:
> > On Mon, Oct 05, 2020 at 09:37:37AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > >
> > > The function scrub_scan_estimate_blocks naïvely uses the statvfs counts
> > > to estimate the size and free blocks on the data volume. Unfortunately,
> > > it fails to account for the fact that statvfs can return the size and
> > > free counts for the realtime volume if the root directory has the
> > > rtinherit flag set, which leads to phase 7 reporting totally absurd
> > > quantities.
> > >
> > > Eric pointed out a further problem with statvfs, which is that the file
> > > counts are clamped to the current user's project quota inode limits.
> > > Therefore, we must not use statvfs for querying the filesystem summary
> > > counts.
> > >
> > > The XFS_IOC_FSCOUNTS ioctl returns all the data we need, so use that
> > > instead.
> > >
> > > Fixes: 604dd3345f35 ("xfs_scrub: filesystem counter collection functions")
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > v2: drop statvfs entirely
> > > ---
> >
> > This doesn't seem to apply to for-next..?
> >
>
> Oops, never mind. Wrong tree...
>
>
> > Brian
> >
> > > scrub/fscounters.c | 27 ++++-----------------------
> > > 1 file changed, 4 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> > > index f9d64f8c008f..e9901fcdf6df 100644
> > > --- a/scrub/fscounters.c
> > > +++ b/scrub/fscounters.c
> > > @@ -130,38 +130,19 @@ scrub_scan_estimate_blocks(
> > > unsigned long long *f_free)
> > > {
> > > struct xfs_fsop_counts fc;
> > > - struct xfs_fsop_resblks rb;
> > > - struct statvfs sfs;
> > > int error;
> > >
> > > - /* Grab the fstatvfs counters, since it has to report accurately. */
> > > - error = fstatvfs(ctx->mnt.fd, &sfs);
> > > - if (error)
> > > - return errno;
> > > -
> > > /* Fetch the filesystem counters. */
> > > error = ioctl(ctx->mnt.fd, XFS_IOC_FSCOUNTS, &fc);
> > > if (error)
> > > return errno;
> > >
> > > - /*
> > > - * XFS reserves some blocks to prevent hard ENOSPC, so add those
> > > - * blocks back to the free data counts.
> > > - */
> > > - error = ioctl(ctx->mnt.fd, XFS_IOC_GET_RESBLKS, &rb);
> > > - if (error)
> > > - return errno;
> > > -
> > > - sfs.f_bfree += rb.resblks_avail;
> > > -
> > > - *d_blocks = sfs.f_blocks;
> > > - if (ctx->mnt.fsgeom.logstart > 0)
> > > - *d_blocks += ctx->mnt.fsgeom.logblocks;
> > > - *d_bfree = sfs.f_bfree;
> > > + *d_blocks = ctx->mnt.fsgeom.datablocks;
> > > + *d_bfree = fc.freedata;
> > > *r_blocks = ctx->mnt.fsgeom.rtblocks;
> > > *r_bfree = fc.freertx;
> > > - *f_files = sfs.f_files;
> > > - *f_free = sfs.f_ffree;
> > > + *f_files = fc.allocino;
> > > + *f_free = fc.freeino;
> > >
>
> Aren't the free inode counters semantically different between statvfs
> and this ioctl? I thought stat had some logic to effectively show free
> data blocks as free inodes,
It does.
> whereas the ioctl looks like it just reads
> our internal counter (which IIRC is a subset of physically allocated
> inode chunks). Do we care about that semantic here either way?
Nope. The one caller that cares (scrub/phase7.c) only wants to know the
number of inodes in use (f_free - f_files), which is unaffected by the
logic in xfs_fs_statfs.
I suppose I could trim the parameter list even further to return only
the file count...
--D
> Brian
>
> > > return 0;
> > > }
> > >
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
2020-10-05 16:37 [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts Darrick J. Wong
2020-10-09 11:18 ` Brian Foster
@ 2020-10-12 19:29 ` Eric Sandeen
1 sibling, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2020-10-12 19:29 UTC (permalink / raw)
To: Darrick J. Wong, Eric Sandeen; +Cc: xfs
On 10/5/20 11:37 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The function scrub_scan_estimate_blocks naïvely uses the statvfs counts
> to estimate the size and free blocks on the data volume. Unfortunately,
> it fails to account for the fact that statvfs can return the size and
> free counts for the realtime volume if the root directory has the
> rtinherit flag set, which leads to phase 7 reporting totally absurd
> quantities.
>
> Eric pointed out a further problem with statvfs, which is that the file
> counts are clamped to the current user's project quota inode limits.
> Therefore, we must not use statvfs for querying the filesystem summary
> counts.
>
> The XFS_IOC_FSCOUNTS ioctl returns all the data we need, so use that
> instead.
>
> Fixes: 604dd3345f35 ("xfs_scrub: filesystem counter collection functions")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> v2: drop statvfs entirely
> ---
> scrub/fscounters.c | 27 ++++-----------------------
> 1 file changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index f9d64f8c008f..e9901fcdf6df 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -130,38 +130,19 @@ scrub_scan_estimate_blocks(
> unsigned long long *f_free)
> {
> struct xfs_fsop_counts fc;
> - struct xfs_fsop_resblks rb;
> - struct statvfs sfs;
> int error;
>
> - /* Grab the fstatvfs counters, since it has to report accurately. */
> - error = fstatvfs(ctx->mnt.fd, &sfs);
> - if (error)
> - return errno;
> -
> /* Fetch the filesystem counters. */
> error = ioctl(ctx->mnt.fd, XFS_IOC_FSCOUNTS, &fc);
> if (error)
> return errno;
>
> - /*
> - * XFS reserves some blocks to prevent hard ENOSPC, so add those
> - * blocks back to the free data counts.
> - */
> - error = ioctl(ctx->mnt.fd, XFS_IOC_GET_RESBLKS, &rb);
> - if (error)
> - return errno;
> -
> - sfs.f_bfree += rb.resblks_avail;
> -
> - *d_blocks = sfs.f_blocks;
> - if (ctx->mnt.fsgeom.logstart > 0)
> - *d_blocks += ctx->mnt.fsgeom.logblocks;
> - *d_bfree = sfs.f_bfree;
> + *d_blocks = ctx->mnt.fsgeom.datablocks;
> + *d_bfree = fc.freedata;
> *r_blocks = ctx->mnt.fsgeom.rtblocks;
> *r_bfree = fc.freertx;
> - *f_files = sfs.f_files;
> - *f_free = sfs.f_ffree;
> + *f_files = fc.allocino;
> + *f_free = fc.freeino;
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts
2020-10-09 15:37 ` Darrick J. Wong
@ 2020-10-15 8:16 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2020-10-15 8:16 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Brian Foster, Eric Sandeen, xfs
On Fri, Oct 09, 2020 at 08:37:41AM -0700, Darrick J. Wong wrote:
> > whereas the ioctl looks like it just reads
> > our internal counter (which IIRC is a subset of physically allocated
> > inode chunks). Do we care about that semantic here either way?
>
> Nope. The one caller that cares (scrub/phase7.c) only wants to know the
> number of inodes in use (f_free - f_files), which is unaffected by the
> logic in xfs_fs_statfs.
>
> I suppose I could trim the parameter list even further to return only
> the file count...
Please do. Or just kill the helper entirely and open code the trivial
ioctl call in the two callers.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-15 8:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 16:37 [PATCH v2] xfs_scrub: don't use statvfs to collect filesystem summary counts Darrick J. Wong
2020-10-09 11:18 ` Brian Foster
2020-10-09 11:32 ` Brian Foster
2020-10-09 15:37 ` Darrick J. Wong
2020-10-15 8:16 ` Christoph Hellwig
2020-10-12 19:29 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).