2009-07-06 05:36:19

by Cong Wang

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

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

Signed-off-by: WANG Cong <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Heiko Carstens <[email protected]>
Cc: Miklos Szeredi <[email protected]>

---
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;
}

2009-07-20 21:12:19

by Andrew Morton

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

On Mon, 6 Jul 2009 01:35:30 -0400
Amerigo Wang <[email protected]> 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?

2009-07-21 10:05:32

by Cong Wang

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

Andrew Morton wrote:
> On Mon, 6 Jul 2009 01:35:30 -0400
> Amerigo Wang <[email protected]> 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? :)

2009-07-21 11:09:45

by Miklos Szeredi

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

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 <[email protected]> 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

2009-07-22 09:19:42

by Cong Wang

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

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 <[email protected]> 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.