Re: [PATCH] loop.c to use write ops for fs requiring special locking

[Robert S Peterson <rpeterso@redhat.com>]

On Tue, 2006-03-28 at 15:40 +0100, Christoph Hellwig wrote:
> NACK.  Adding random flags just makes the kernel unmaintainble.  The

Wrong on two accounts:  (1) This is NOT a random flag and (2) Adding
flags in fact can increase the maintainability of the kernel.  
With this new flag, other kernel drivers may now be written that
better understand the special needs of the underlying filesystem
and act accordingly.  Would you rather have modules like loop.c
special-case each underlying fs?  That's just plain ugly.  As in:

if underlying == ext2 { do this }
else if underlying == vfat { do that }
else if underlying == reiserfs { do the other }
etc.

> right thing is to define a proper highlevel interface that can be
> implemented properly on all filesystems plus a library helper for
> normal pagecache-based filesystems using the aops.  There's already
> various in-kernel filesystems that would require additional locking
> or that aren't pagecache-based at all, please fix them up as part
> of the patch(-series).

This is way out of the scope of the problem.  The problem is that
loop.c circumvents proper locking by going directly to prepare_write
rather than following the normal process.  If I added some kind of
library callbacks to allow cluster locking, it would break the
normal locking sequence of an ordinary write.

The normal sequence of an ordinary write typically looks like:

1. write
2.   (Take care of cluster locking if necessary)
3.   generic_file_write_nolock
4.      generic_file_aio_write_nolock
5.         generic_file_buffered_write
6.            a_ops->prepare_write
etc.

Right now, loop.c is circumventing step 2 (optional cluster locking
if needed by the underlying fs) by bypassing the "write" op.  If I 
added callbacks to prepare_write at (7) as you suggest, it would either
(a) introduce deadlocks conflicting with step 2 above or
(b) require the underlying fs to use its own versions of 3,4,5, and 6,
thus bypassing a great deal of vfs, which is not good for anyone.

Some could argue that loop.c should always use write rather than
prepare_write/commit_write, but most fs's don't have special
locking needs and therefore using them is a performance boost.
So why punish all fs's because of special locking needs require by
a few when we can simply communicate this need with a constant?

Bob Peterson
Red Hat Cluster Suite