Re: [patch 22/23] fs: check for statfs overflow

[Nishanth Aravamudan <nacc@us.ibm.com>]

On 29.05.2008 [17:56:07 -0600], Andreas Dilger wrote:
> On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> > fs: check for statfs overflow
> > 
> > Adds a check for an overflow in the filesystem size so if someone is
> > checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
> > will report back EOVERFLOW instead of a size of 0.
> > 
> > Are other places that need a similar check?  I had tried a similar
> > check in put_compat_statfs64 too but it didn't seem to generate an
> > EOVERFLOW in my test case.
> > 
> > Signed-off-by: Jon Tollefson <kniht@linux.vnet.ibm.com>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ---
> > 
> >  fs/compat.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > Index: linux-2.6/fs/compat.c
> > ===================================================================
> > --- linux-2.6.orig/fs/compat.c
> > +++ linux-2.6/fs/compat.c
> > @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
> >  {
> >  	
> >  	if (sizeof ubuf->f_blocks == 4) {
> > -		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail) &
> > -		    0xffffffff00000000ULL)
> > +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> > +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
> >  			return -EOVERFLOW;
> 
> Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
> nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
> f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
> statfs structure.

Being a FS newbie, I'm not entirely sure I follow, could you say that
again in patch-form? :) Seriously, it might make it clear to me.

> We did this for several years with Lustre, as the first installation
> was already larger than 16TB on 32-bit clients at the time.  There was
> never a problem with statfs returning a larger f_bsize, since
> applications generally use the fstat() st_blocksize to determine IO
> size and not the statfs() data.

I'm not sure that's a good reason to give bad data back to userspace...
We have both interfaces and both should work?

> Returning statfs data accurate to within a few kB is better than
> failing the request outright, IMHO.

Well, currently (iirc), we see statfs() give bad values for 16gb
hugetlbfs mountpoints. That's not good, and is inconsistent with the
other hugetlbfs mountpoints. We actually do want to indicate EOVERFLOW
there, as the 32-bit binary, or some kind of error, although the binary
will notice it can't use the pages from that mountpoint when mmap()
fails :)

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center