[Patch] pipe: use file_update_time() when hold i_mutex

[Patch] pipe: use file_update_time() when hold i_mutex

[Amerigo Wang]

file_update_time() should be called with i_mutex held,
move it before mutex_unlock().

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Miklos Szeredi <mszeredi@suse.cz>

---
diff --git a/fs/pipe.c b/fs/pipe.c
index f7dd21a..724b035 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -602,13 +602,14 @@ redo2:
 		pipe->waiting_writers--;
 	}
 out:
+	if (ret > 0)
+		file_update_time(filp);
+
 	mutex_unlock(&inode->i_mutex);
 	if (do_wakeup) {
 		wake_up_interruptible_sync(&pipe->wait);
 		kill_fasync(&pipe->fasync_readers, SIGIO, POLL_IN);
 	}
-	if (ret > 0)
-		file_update_time(filp);
 	return ret;
 }
 

Re: [Patch] pipe: use file_update_time() when hold i_mutex

[Andrew Morton]

On Mon, 6 Jul 2009 01:35:30 -0400
Amerigo Wang <amwang@redhat.com> wrote:

> 
> file_update_time() should be called with i_mutex held,
> move it before mutex_unlock().
> 

Why do you believe that file_update_time() needs i_mutex?

Re: [Patch] pipe: use file_update_time() when hold i_mutex

[Amerigo Wang]

Andrew Morton wrote:
> On Mon, 6 Jul 2009 01:35:30 -0400
> Amerigo Wang <amwang@redhat.com> wrote:
>
>   
>> file_update_time() should be called with i_mutex held,
>> move it before mutex_unlock().
>>
>>     
>
> Why do you believe that file_update_time() needs i_mutex?
>   
file_update_time() modifies inode, no? :)

Re: [Patch] pipe: use file_update_time() when hold i_mutex

[Miklos Szeredi]

On Tue, 2009-07-21 at 18:07 +0800, Amerigo Wang wrote:
> Andrew Morton wrote:
> > On Mon, 6 Jul 2009 01:35:30 -0400
> > Amerigo Wang <amwang@redhat.com> wrote:
> >
> >   
> >> file_update_time() should be called with i_mutex held,
> >> move it before mutex_unlock().
> >>
> >>     
> >
> > Why do you believe that file_update_time() needs i_mutex?
> >   
> file_update_time() modifies inode, no? :)

So does touch_atime(), yet neither needs i_mutex.

But calling file_update_time() within the locked region in pipe_write()
might make sense regardless: that way a task waiting for data would be
guaranteed to see the time change after receiving a POLLIN event for
example.

Thanks,
Miklos

Re: [Patch] pipe: use file_update_time() when hold i_mutex

[Amerigo Wang]

Miklos Szeredi wrote:
> On Tue, 2009-07-21 at 18:07 +0800, Amerigo Wang wrote:
>   
>> Andrew Morton wrote:
>>     
>>> On Mon, 6 Jul 2009 01:35:30 -0400
>>> Amerigo Wang <amwang@redhat.com> wrote:
>>>
>>>   
>>>       
>>>> file_update_time() should be called with i_mutex held,
>>>> move it before mutex_unlock().
>>>>
>>>>     
>>>>         
>>> Why do you believe that file_update_time() needs i_mutex?
>>>   
>>>       
>> file_update_time() modifies inode, no? :)
>>     
>
> So does touch_atime(), yet neither needs i_mutex.
>   
Yes?

Then how the inode is protected when file_update_time() modifies
it?

Thanks.

Re: [Patch] pipe: use file_update_time() when hold i_mutex

[Miklos Szeredi]

On Wed, 2009-07-22 at 17:21 +0800, Amerigo Wang wrote:
> Miklos Szeredi wrote:
> > On Tue, 2009-07-21 at 18:07 +0800, Amerigo Wang wrote:
> >   
> >> Andrew Morton wrote:
> >>     
> >>> On Mon, 6 Jul 2009 01:35:30 -0400
> >>> Amerigo Wang <amwang@redhat.com> wrote:
> >>>
> >>>   
> >>>       
> >>>> file_update_time() should be called with i_mutex held,
> >>>> move it before mutex_unlock().
> >>>>
> >>>>     
> >>>>         
> >>> Why do you believe that file_update_time() needs i_mutex?
> >>>   
> >>>       
> >> file_update_time() modifies inode, no? :)
> >>     
> >
> > So does touch_atime(), yet neither needs i_mutex.
> >   
> Yes?
> 
> Then how the inode is protected when file_update_time() modifies
> it?

Protected against what?

For example the inode isn't protected against concurrent access by
stat(2), since that doesn't take i_mutex.

And indeed it looks like there are races there, since set/get operations
are not atomic on the timestamp, so stat could return a garbled value if
it races with file_update_time() or touch_atime().

Possibly worth fixing, but I'm not sure how to do that without affecting
the scalability of stat and friends and without adding too much
complexity.

Thanks,
Miklos