[09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
diff mbox

Message ID 20140206155029.b7275670913c34ef6bc9a530@linux-foundation.org
State New, archived
Headers show

Commit Message

Andrew Morton Feb. 6, 2014, 11:50 p.m. UTC
On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:

> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
> > From: Jensen <shencanquan@huawei.com>
> > Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
> > 
> > llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
> > because the file size maybe update on another node.
> > 
> > This bug can be reproduce the following scenario: at first, we dd a test
> > fileA, the file size is 10k.
> 
> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
> right way to do it.

Sunil was worried about the performance impact.  Correctness beats
performance, but some quantitative testing would be useful?

> Reviewed-by: Mark Fasheh <mfasheh@suse.de>

I suppose we should fix the bug it added.

Comments

Andrew Morton Feb. 6, 2014, 11:53 p.m. UTC | #1
On Thu, 6 Feb 2014 15:50:29 -0800 Andrew Morton <akpm@linux-foundation.org> wrote:

> I suppose we should fix the bug it added.
> 
> --- a/fs/ocfs2/file.c~ocfs2-llseek-requires-ocfs2-inode-lock-for-the-file-in-seek_end-fix
> +++ a/fs/ocfs2/file.c
> @@ -2631,6 +2631,7 @@ static loff_t ocfs2_file_llseek(struct f
>  		 */
>  		ret = ocfs2_inode_lock(inode, NULL, 0);
>  		if (ret < 0) {
> +			ocfs2_inode_unlock(inode, 0);
>  			mlog_errno(ret);
>  			goto out;
>  		}

oops, scratch that.
Mark Fasheh Feb. 7, 2014, 10:44 p.m. UTC | #2
On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
> 
> > On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
> > > From: Jensen <shencanquan@huawei.com>
> > > Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
> > > 
> > > llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
> > > because the file size maybe update on another node.
> > > 
> > > This bug can be reproduce the following scenario: at first, we dd a test
> > > fileA, the file size is 10k.
> > 
> > Basically, you want to amke SEEK_END cluster-aware. This patch would be the
> > right way to do it.
> 
> Sunil was worried about the performance impact.  Correctness beats
> performance, but some quantitative testing would be useful?

Performance is my primary concern as well. I thought of writing it up but
realized I don't really have any evidence off the top of my head one way or
the other that this might slow us down.

That said, I kind of question the usefulness of this patch - we got
along pretty well so far without locking in lseek and some random dd(1) test
doesn't really provide a great end-user reason for why we should do this.

I will note that gfs2 locks for SEEK_END.


Testing would help to answer this, yes. Jensen is this something you can do?
I'm not sure exactly what we would run yet though, I have to think about
that (or maybe someone can suggest something).

Thanks,
	--Mark


--
Mark Fasheh
Shen Canquan Feb. 8, 2014, 1:26 a.m. UTC | #3
On 2014/2/8 6:44, Mark Fasheh wrote:
> On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
>> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
>>
>>> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
>>>> From: Jensen <shencanquan@huawei.com>
>>>> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
>>>>
>>>> llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
>>>> because the file size maybe update on another node.
>>>>
>>>> This bug can be reproduce the following scenario: at first, we dd a test
>>>> fileA, the file size is 10k.
>>> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
>>> right way to do it.
>> Sunil was worried about the performance impact.  Correctness beats
>> performance, but some quantitative testing would be useful?
> Performance is my primary concern as well. I thought of writing it up but
> realized I don't really have any evidence off the top of my head one way or
> the other that this might slow us down.
>
> That said, I kind of question the usefulness of this patch - we got
> along pretty well so far without locking in lseek and some random dd(1) test
> doesn't really provide a great end-user reason for why we should do this.
>
> I will note that gfs2 locks for SEEK_END.
>
>
> Testing would help to answer this, yes. Jensen is this something you can do?
> I'm not sure exactly what we would run yet though, I have to think about
> that (or maybe someone can suggest something).
>
> Thanks,
> 	--Mark
>
   ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
   cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.

  Thanks,
                  --Jensen

> --
> Mark Fasheh
>
> .
>
Mark Fasheh Feb. 8, 2014, 2:07 a.m. UTC | #4
On Sat, Feb 08, 2014 at 09:26:03AM +0800, Jensen wrote:
> On 2014/2/8 6:44, Mark Fasheh wrote:
> > On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
> >> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
> >>
> >>> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
> >>>> From: Jensen <shencanquan@huawei.com>
> >>>> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
> >>>>
> >>>> llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
> >>>> because the file size maybe update on another node.
> >>>>
> >>>> This bug can be reproduce the following scenario: at first, we dd a test
> >>>> fileA, the file size is 10k.
> >>> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
> >>> right way to do it.
> >> Sunil was worried about the performance impact.  Correctness beats
> >> performance, but some quantitative testing would be useful?
> > Performance is my primary concern as well. I thought of writing it up but
> > realized I don't really have any evidence off the top of my head one way or
> > the other that this might slow us down.
> >
> > That said, I kind of question the usefulness of this patch - we got
> > along pretty well so far without locking in lseek and some random dd(1) test
> > doesn't really provide a great end-user reason for why we should do this.
> >
> > I will note that gfs2 locks for SEEK_END.
> >
> >
> > Testing would help to answer this, yes. Jensen is this something you can do?
> > I'm not sure exactly what we would run yet though, I have to think about
> > that (or maybe someone can suggest something).
> >
> > Thanks,
> > 	--Mark
> >
>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.

That doesn't mean we can't or shouldn't quantify the performance impact of your patch.

Please help us measure what the end-user impact of this change will be.
	--Mark

--
Mark Fasheh
jeff.liu Feb. 8, 2014, 2:24 a.m. UTC | #5
Hi Canquan,

On 02/08 2014 09:26 AM, Jensen wrote:
> On 2014/2/8 6:44, Mark Fasheh wrote:
>> On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
>>> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
>>>
>>>> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
>>>>> From: Jensen <shencanquan@huawei.com>
>>>>> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
>>>>>
>>>>> llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
>>>>> because the file size maybe update on another node.
>>>>>
>>>>> This bug can be reproduce the following scenario: at first, we dd a test
>>>>> fileA, the file size is 10k.
>>>> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
>>>> right way to do it.
>>> Sunil was worried about the performance impact.  Correctness beats
>>> performance, but some quantitative testing would be useful?
>> Performance is my primary concern as well. I thought of writing it up but
>> realized I don't really have any evidence off the top of my head one way or
>> the other that this might slow us down.
>>
>> That said, I kind of question the usefulness of this patch - we got
>> along pretty well so far without locking in lseek and some random dd(1) test
>> doesn't really provide a great end-user reason for why we should do this.
>>
>> I will note that gfs2 locks for SEEK_END.
>>
>>
>> Testing would help to answer this, yes. Jensen is this something you can do?
>> I'm not sure exactly what we would run yet though, I have to think about
>> that (or maybe someone can suggest something).
>>
>> Thanks,
>> 	--Mark
>>
>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.

That would be great if you can show any testing results regarding performance
as you're the author of this patch and if you'd like to continue to push it,
though this is not requested always.


Thanks,
-Jeff
Shen Canquan Feb. 8, 2014, 2:46 a.m. UTC | #6
On 2014/2/8 10:07, Mark Fasheh wrote:
> On Sat, Feb 08, 2014 at 09:26:03AM +0800, Jensen wrote:
>> On 2014/2/8 6:44, Mark Fasheh wrote:
>>> On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
>>>> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
>>>>
>>>>> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
>>>>>> From: Jensen <shencanquan@huawei.com>
>>>>>> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
>>>>>>
>>>>>> llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
>>>>>> because the file size maybe update on another node.
>>>>>>
>>>>>> This bug can be reproduce the following scenario: at first, we dd a test
>>>>>> fileA, the file size is 10k.
>>>>> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
>>>>> right way to do it.
>>>> Sunil was worried about the performance impact.  Correctness beats
>>>> performance, but some quantitative testing would be useful?
>>> Performance is my primary concern as well. I thought of writing it up but
>>> realized I don't really have any evidence off the top of my head one way or
>>> the other that this might slow us down.
>>>
>>> That said, I kind of question the usefulness of this patch - we got
>>> along pretty well so far without locking in lseek and some random dd(1) test
>>> doesn't really provide a great end-user reason for why we should do this.
>>>
>>> I will note that gfs2 locks for SEEK_END.
>>>
>>>
>>> Testing would help to answer this, yes. Jensen is this something you can do?
>>> I'm not sure exactly what we would run yet though, I have to think about
>>> that (or maybe someone can suggest something).
>>>
>>> Thanks,
>>> 	--Mark
>>>
>>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
>>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.
> That doesn't mean we can't or shouldn't quantify the performance impact of your patch.
>
> Please help us measure what the end-user impact of this change will be.
> 	--Mark
Ok,  I see.  I think that how to test the performance impact.  we found the bug in web application which only concerned about the correctness.
Thanks,
                    --Jensen
Shen Canquan Feb. 10, 2014, 8:51 a.m. UTC | #7
On 2014/2/8 10:07, Mark Fasheh wrote:
> On Sat, Feb 08, 2014 at 09:26:03AM +0800, Jensen wrote:
>> On 2014/2/8 6:44, Mark Fasheh wrote:
>>> On Thu, Feb 06, 2014 at 03:50:29PM -0800, Andrew Morton wrote:
>>>> On Thu, 6 Feb 2014 15:42:53 -0800 Mark Fasheh <mfasheh@suse.de> wrote:
>>>>
>>>>> On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm@linux-foundation.org wrote:
>>>>>> From: Jensen <shencanquan@huawei.com>
>>>>>> Subject: ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END
>>>>>>
>>>>>> llseek requires ocfs2 inode lock for updating the file size in SEEK_END. 
>>>>>> because the file size maybe update on another node.
>>>>>>
>>>>>> This bug can be reproduce the following scenario: at first, we dd a test
>>>>>> fileA, the file size is 10k.
>>>>> Basically, you want to amke SEEK_END cluster-aware. This patch would be the
>>>>> right way to do it.
>>>> Sunil was worried about the performance impact.  Correctness beats
>>>> performance, but some quantitative testing would be useful?
>>> Performance is my primary concern as well. I thought of writing it up but
>>> realized I don't really have any evidence off the top of my head one way or
>>> the other that this might slow us down.
>>>
>>> That said, I kind of question the usefulness of this patch - we got
>>> along pretty well so far without locking in lseek and some random dd(1) test
>>> doesn't really provide a great end-user reason for why we should do this.
>>>
>>> I will note that gfs2 locks for SEEK_END.
>>>
>>>
>>> Testing would help to answer this, yes. Jensen is this something you can do?
>>> I'm not sure exactly what we would run yet though, I have to think about
>>> that (or maybe someone can suggest something).
>>>
>>> Thanks,
>>> 	--Mark
>>>
>>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
>>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.
> That doesn't mean we can't or shouldn't quantify the performance impact of your patch.
>
> Please help us measure what the end-user impact of this change will be.
> 	--Mark
  test result: 1000000 times lseek call;
  index        lseek with inode lock (unit:us)                lseek without inode lock (unit:us)
    1                   1168162                                    555383
    2                   1168011                                    549504
    3                   1170538                                    549396
    4                   1170375                                    551685
    5                   1170444                                    556719
    6                   1174364                                    555307
    7                   1163294                                    551552
    8                   1170080                                    549350
    9                   1162464                                    553700
  10                   1165441                                    552594

 avg                  1168317                                    552519

avg with lock - avg without lock = 615798
(avg with lock - avg without lock)/1000000=0.615798 us

>
> --
> Mark Fasheh
>
> .
>
Andrew Morton Feb. 10, 2014, 11:57 p.m. UTC | #8
On Mon, 10 Feb 2014 16:51:32 +0800 Jensen <shencanquan@huawei.com> wrote:

> >>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
> >>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.
> > That doesn't mean we can't or shouldn't quantify the performance impact of your patch.
> >
> > Please help us measure what the end-user impact of this change will be.
> > 	--Mark
>   test result: 1000000 times lseek call;
>   index        lseek with inode lock (unit:us)                lseek without inode lock (unit:us)
>     1                   1168162                                    555383
>     2                   1168011                                    549504
>     3                   1170538                                    549396
>     4                   1170375                                    551685
>     5                   1170444                                    556719
>     6                   1174364                                    555307
>     7                   1163294                                    551552
>     8                   1170080                                    549350
>     9                   1162464                                    553700
>   10                   1165441                                    552594
> 
>  avg                  1168317                                    552519
> 
> avg with lock - avg without lock = 615798
> (avg with lock - avg without lock)/1000000=0.615798 us

hm, what does that actually mean.  I guess that to get a feel for the
impact on real workloads, this latency needs to be compared with the
time for a small IO on fast hardware.

One could test that by creating a large file then doing lots of
"lseek(random)+read" operations.  Which requires forgetting about
the existence of pread() ;)
Shen Canquan Feb. 11, 2014, 3:35 a.m. UTC | #9
On 2014/2/11 7:57, Andrew Morton wrote:
> On Mon, 10 Feb 2014 16:51:32 +0800 Jensen <shencanquan@huawei.com> wrote:
>
>>>>    ocfs2 is a cluster file system.  as like read/write/open/rmdir/unlink interface which think of cluster-aware. I think the seek interface need
>>>>    cluster-aware. May be it has the performance impact. but it's correctness is more important than performance.
>>> That doesn't mean we can't or shouldn't quantify the performance impact of your patch.
>>>
>>> Please help us measure what the end-user impact of this change will be.
>>> 	--Mark
>>   test result: 1000000 times lseek call;
>>   index        lseek with inode lock (unit:us)                lseek without inode lock (unit:us)
>>     1                   1168162                                    555383
>>     2                   1168011                                    549504
>>     3                   1170538                                    549396
>>     4                   1170375                                    551685
>>     5                   1170444                                    556719
>>     6                   1174364                                    555307
>>     7                   1163294                                    551552
>>     8                   1170080                                    549350
>>     9                   1162464                                    553700
>>   10                   1165441                                    552594
>>
>>  avg                  1168317                                    552519
>>
>> avg with lock - avg without lock = 615798
>> (avg with lock - avg without lock)/1000000=0.615798 us
> hm, what does that actually mean.  I guess that to get a feel for the
> impact on real workloads, this latency needs to be compared with the
> time for a small IO on fast hardware.
>
> One could test that by creating a large file then doing lots of
> "lseek(random)+read" operations.  Which requires forgetting about
> the existence of pread() ;)
>
> .
   because lseek only lock in SEEK_END.  so your above mention can't test.

>

Patch
diff mbox

--- a/fs/ocfs2/file.c~ocfs2-llseek-requires-ocfs2-inode-lock-for-the-file-in-seek_end-fix
+++ a/fs/ocfs2/file.c
@@ -2631,6 +2631,7 @@  static loff_t ocfs2_file_llseek(struct f
 		 */
 		ret = ocfs2_inode_lock(inode, NULL, 0);
 		if (ret < 0) {
+			ocfs2_inode_unlock(inode, 0);
 			mlog_errno(ret);
 			goto out;
 		}