Re: [RFC/PATCH] - revert generic_fillattr stat->blksize toPAGE_CACHE_SIZE

[Dave Kleikamp]

On Mon, 2006-11-06 at 15:50 -0600, Eric Sandeen wrote:
> The inode diet patches first did this to generic_fillattr():
> 
> -	stat->blksize = inode->i_blksize;
> +	stat->blksize = PAGE_CACHE_SIZE;
> 
> but by 2.6.19-rc3 this was changed again:
> 
> -	stat->blksize = PAGE_CACHE_SIZE;
> +	stat->blksize = (1 << inode->i_blkbits);
> 
> I can't find for sure why this was done; perhaps because it exposed a bug
> in cifs[1]

Steve has fixed the bug in cifs_readdir().

> However, if we are going to pick a default for generic_fillattr, it should probably
> be what most filesystems were using before, and let filesystems which need something
> else re-set it to according to their needs.  As it stands today, doing readdirs and
> the like in block-sized chunks rather than page sized will probably not be the
> best thing for performance.

I agree.

> 
> so I would propose the following patch to make PAGE_CACHE_SIZE the default (again), 
> and let filesystems which need something -else- do that on their own.
> 
> [1] http://lists.samba.org/archive/linux-cifs-client/2006-September/001481.html
> 
> Thanks,
> -Eric
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> 
> --- linux.orig/fs/stat.c	2006-11-05 23:27:04.962482569 -0600
> +++ linux/fs/stat.c	2006-11-05 23:27:29.394396050 -0600
> @@ -33,7 +33,7 @@
>   	stat->ctime = inode->i_ctime;
>   	stat->size = i_size_read(inode);
>   	stat->blocks = inode->i_blocks;
> -	stat->blksize = (1 << inode->i_blkbits);
> +	stat->blksize = PAGE_CACHE_SIZE;
>   }
> 
>   EXPORT_SYMBOL(generic_fillattr);

Looks good.  Since cifs is affected by this patch, I propose that cifs
explicitly set stat->blksize:

CIFS: Explicitly set stat->blksize

CIFS may perform I/O over the network in larger chunks than the page size,
so it should explicitly set stat->blksize to ensure optimal I/O bandwidth

Signed-off-by: Dave Kleikamp <shaggy@xxxxxxxxxxxxxxxxxx>

diff -Nurp linux.orig/fs/cifs/inode.c linux/fs/cifs/inode.c
--- linux.orig/fs/cifs/inode.c	2006-11-03 13:44:04.000000000 -0600
+++ linux/fs/cifs/inode.c	2006-11-06 16:11:21.000000000 -0600
@@ -1089,8 +1089,10 @@ int cifs_getattr(struct vfsmount *mnt, s
 	struct kstat *stat)
 {
 	int err = cifs_revalidate(dentry);
-	if (!err)
+	if (!err) {
 		generic_fillattr(dentry->d_inode, stat);
+		stat->blksize = CIFS_MAX_MSGSIZE;
+	}
 	return err;
 }
 

-- 
David Kleikamp
IBM Linux Technology Center

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/