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

[Christoph Hellwig <hch@infradead.org>]

On Tue, Mar 28, 2006 at 09:59:17AM -0600, Robert S Peterson wrote:
> 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.

adding flags adds special cases.  in this particular case it adds a special
case to hack around a leaking abstraction.  the right thing is to fix that
leaky abstraction as I said in my previous mail.  please go ahead and add 
a proper abstraction at the file operation level that
gets rid of this leaky abstraction instead of adding a kludge ontop of an
existing one.

> 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.

such crap might be acceptable inside redhat, but in kernel land it never
was so this never would be even considered an option.

> 
> > 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.

no it's not out of scope.  we prefer to do things right, and kludging
a nother hack ontop of a historical accident is not right.  and given
that you care about getting rid of the old wart we have pretty nice
power to get you to do the right thing or nothing at all.

> 
> The normal sequence of an ordinary write typically looks like:

thanks, I'm not stupid, in fact I've writen quite a bit of that code.

please don't try to tell me the obvious things and go and fix the thing.