Message-Id: <6.0.0.20.2.20090116105626.058e0768@172.19.0.2>
Date:	Fri, 16 Jan 2009 11:08:03 +0900
From:	Hisashi Hifumi <hifumi.hisashi@....ntt.co.jp>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Theodore Tso <tytso@....edu>, matthew@....cx,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RESEND] [PATCH] lseek: change i_mutex usage.


At 10:49 09/01/16, Andrew Morton wrote:
>On Fri, 16 Jan 2009 09:53:02 +0900 Hisashi Hifumi 
><hifumi.hisashi@....ntt.co.jp> wrote:
>
>> 
>> At 09:40 09/01/16, Andrew Morton wrote:
>> >On Thu, 15 Jan 2009 09:21:13 -0500
>> >Theodore Tso <tytso@....edu> wrote:
>> >
>> >> On Thu, Jan 15, 2009 at 06:22:52AM -0700, Matthew Wilcox wrote:
>> >> > 
>> >> > Of course if you have multiple threads, they will share a struct file,
>> >> > and you're updating f_pos and f_version without locking.  Maybe that's
>> >> > OK, but it's soemthing you didn't discuss.
>> >> 
>> >> f_pos is updated by sys_write(), and friends without locking, so we're
>> >> fine on that front, or at least no worse off.
>> >
>> >bug ;)
>> >
>> >>  SUSv3 doesn't seem to
>> >> say one way or another what should happen if two threads try to
>> >> write() to a file at the same time using the same file descriptor in
>> >> terms of whether or not f_pos gets updated intelligently.  We've opted
>> >> for speed over determinism already.
>> >
>> >I think our thinking was that if two threads are racily updating f_pos
>> >with different values, then it should end up with one of those values.
>> >
>> >From a quality-of-implementation POV (what _is_ that, anyway) it would
>> >be bad if the kernel were to set f_pos to the upper 32 bits of position
>> >A and the lower 32 bits of position B.  Which could happen if we remove
>> >the i_mutex protection on 32-bits.
>> >
>> >We could perhaps omit some locking if CONFIG_64BIT.  There's probably
>> >quite a bit of locking which could be omitted in that case.
>> 
>> Updating f_pos value on 32bit is not atomic, so we discussed about this
>> but we concluded that it does not matter whether f_pos is atomic or not
>
>It's unclear what you're saying here.
>
>I see three issues here:
>
>a) two racing threads update f_pos.  One of them wins, and the
>   outcome in indeterminate.
>
>b) two racing threads update f_pos and the end result is that f_pos
>   contains a value which *neither* thread tried to write.
>
>c) one thread is writing and the other reading.  There is a window
>   where the reader can see an intermediate value which is a mix of the
>   old and new values.
>
>I think we decided that a) is acceptable, b) is not and that c) can only
>occur on multiple-of-4G wraparounds and isn't worth bothering about.
>
>> See,
>> Subject:[RESEND] [PATCH] VFS: make file->f_pos access atomic on 32bit
>> http://marc.info/?l=linux-fsdevel&m=122335627224515
>
>Sorry, I'm disinclined to re-read a long thread, trying to work out
>which bit you might be referring to.

Following is Linus's post about this issue.
http://marc.info/?l=linux-kernel&m=122356445226680&w=2

If we decide that a) is ok and we mind f_pos value being atomic on 32bit arch,
we should use seq_counter to f_pos.

>
>> I think even i_mutex is not needed. When we touch i_size, i_size_read is 
>enough,
>> and we can remove i_mutex at all on lseek.
>
>Why are we talking about i_size now?
>
>Confused.

When caller of lseek set SEEK_END, i_size is referenced. I mentioned about this.
I thought that the reason of i_mutex existence on lseek is only touching i_size.
So if i_size_read is used to touch i_size value, i_mutex could be removed.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists