Re: i_mutex locking in generic_file_splice_write()

[Jens Axboe <jens.axboe@oracle.com>]

On Thu, Oct 12 2006, Mark Fasheh wrote:
> On Thu, Oct 12, 2006 at 12:54:09PM -0700, Andrew Morton wrote:
> > > Shouldn't we be taking this before calling into ->prepare_write() and
> > > ->commit_write(). What's preventing generic_file_splice_write() from racing
> > > a truncate? Or maybe even another write?
> > 
> > The lock_page() will block truncate and will block write()s to this particular
> > page.
> Ok.
> 
> 
> > > A quick look through other callers reveals that generic_file_aio_write() and
> > > do_lo_send_aops() both are careful to take i_mutex.
> > 
> > I'm trying to remember what i_mutex actually protects in this context. 
> > i_size, certainly - if we go changing the file size without locks then
> > other places might get surprised.  For example, a concurrent write() at a
> > larger file offset might try to increase i_size but if it loses the race
> > against the unlocked i_size-changing thread, the inode ends up with the
> > smaller i_size.
> I'm also worried about concurrent allocation tree changes. Perhaps I'm
> mistaken and all file systems we care about can handle them happening
> concurrently, but otherwise couldn't two processes writing to different
> sparse regions in a file cause problems? One process via file write and the
> other via a splice write.
> 
> 
> > So yup, we need i_mutex if only for that reason.
> Ok. Here's a first pass. The double lock is ugly, but as far as I can tell
> we need it. Unless there's a rule about ordering between pipe inodes and
> "other" inodes that I don't know about.
> 
> Compile tested only. I probably won't get a chance to actually run it until
> late this weekend at the earliest :/

Patch looks ok to me. The double lock test only works, as long as splice
is the only one ever locking both mutexes. Or if others follow the same
ordering rules. I'm not very well versed in vfs matters, is that
guarenteed?

-- 
Jens Axboe