On Wed, Sep 16, 2020 at 03:14:52PM +0000, Trond Myklebust wrote:
> On Thu, 2020-09-10 at 20:06 +0000, Frank van der Linden wrote:
> > nfs_statfs rounds up the numbers of blocks as computed
> > from the numbers the server return values.
> >
> > This works out well if the client block size, which is
> > the same as wrsize, is smaller than or equal to the actual
> > filesystem block size on the server.
> >
> > But, for NFS4, the size is usually larger (1M), meaning
> > that statfs reports more free space than actually is
> > available. This confuses, for example, fstest generic/103.
> >
> > Given a choice between reporting too much or too little
> > space, the latter is the safer option, so don't round
> > up the number of blocks. This also simplifies the code.
> >
> > Signed-off-by: Frank van der Linden <fllinden@xxxxxxxxxx>
> > ---
> > fs/nfs/super.c | 25 ++++---------------------
> > 1 file changed, 4 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index 7a70287f21a2..45d368a5cc95 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -217,8 +217,6 @@ EXPORT_SYMBOL_GPL(nfs_client_for_each_server);
> > int nfs_statfs(struct dentry *dentry, struct kstatfs *buf)
> > {
> > struct nfs_server *server = NFS_SB(dentry->d_sb);
> > - unsigned char blockbits;
> > - unsigned long blockres;
> > struct nfs_fh *fh = NFS_FH(d_inode(dentry));
> > struct nfs_fsstat res;
> > int error = -ENOMEM;
> > @@ -241,26 +239,11 @@ int nfs_statfs(struct dentry *dentry, struct
> > kstatfs *buf)
> >
> > buf->f_type = NFS_SUPER_MAGIC;
> >
> > - /*
> > - * Current versions of glibc do not correctly handle the
> > - * case where f_frsize != f_bsize. Eventually we want to
> > - * report the value of wtmult in this field.
> > - */
> > - buf->f_frsize = dentry->d_sb->s_blocksize;
>
> NACK. This comment and explicit setting is there to document why we're
> not propagating the true value of the filesystem block size. Please do
> not remove it.
It's a comment from 2006, which is a bit outdated. wtmult is an NFSv3
value, and glibc hasn't had that issue for a while. Maybe the comment
should be updated? I'm happy to not touch it, though - will leave that
change out.
>
> > -
> > - /*
> > - * On most *nix systems, f_blocks, f_bfree, and f_bavail
> > - * are reported in units of f_frsize. Linux hasn't had
> > - * an f_frsize field in its statfs struct until recently,
> > - * thus historically Linux's sys_statfs reports these
> > - * fields in units of f_bsize.
> > - */
> > buf->f_bsize = dentry->d_sb->s_blocksize;
> > - blockbits = dentry->d_sb->s_blocksize_bits;
> > - blockres = (1 << blockbits) - 1;
> > - buf->f_blocks = (res.tbytes + blockres) >> blockbits;
> > - buf->f_bfree = (res.fbytes + blockres) >> blockbits;
> > - buf->f_bavail = (res.abytes + blockres) >> blockbits;
> > +
> > + buf->f_blocks = res.tbytes / buf->f_bsize;
> > + buf->f_bfree = res.fbytes / buf->f_bsize;
> > + buf->f_bavail = res.abytes / buf->f_bsize;
> >
> > buf->f_files = res.tfiles;
> > buf->f_ffree = res.afiles;
>
> As far as I can tell, all we're doing here is changing rounding up to
> rounding down, since dentry->d_sb->s_blocksize should always equal to
> (1 << dentry->d_sb->s_blocksize_bits).
>
> Otherwise, switching from a shift to division seems like it is just
> making the calculation less efficient. Why?
Well, my thinking here was that using a straight division made the code
simpler, and it's not a code path that is performance-sensitive.
But, I forgot about needing a div64 macro for 32bit archs anyway, which
kind of undoes the 'make the code simpler' argument a bit..
I'll change it to just rounding down (e.g. remove blockres from the
equation).
Thanks,
- Frank
