[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[akpm at linux-foundation.org]

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.

on NodeA:
---------
1) open the test fileA, lseek the end of file. and print the position.
2) close the test fileA

on NodeB:
1) open the test fileA, append the 5k data to test FileA.
2) lseek the end of file. and print the position.
3) close file.

At first we run the test program1 on NodeA , the result is 10k.  And then
run the test program2 on NodeB, the result is 15k.  At last, we run the
test program1 on NodeA again, the result is 10k.

After applying this patch the three step result is 15k.

Signed-off-by: Jensen <shencanquan@huawei.com>
Cc: Jie Liu <jeff.liu@oracle.com>
Acked-by: Joel Becker <jlbec@evilplan.org>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Sunil Mushran <sunil.mushran@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/file.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff -puN fs/ocfs2/file.c~ocfs2-llseek-requires-ocfs2-inode-lock-for-the-file-in-seek_end fs/ocfs2/file.c
--- a/fs/ocfs2/file.c~ocfs2-llseek-requires-ocfs2-inode-lock-for-the-file-in-seek_end
+++ a/fs/ocfs2/file.c
@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct f
 	case SEEK_SET:
 		break;
 	case SEEK_END:
-		offset += inode->i_size;
+		/* SEEK_END requires the OCFS2 inode lock for the file
+		 * because it references the file's size.
+		 */
+		ret = ocfs2_inode_lock(inode, NULL, 0);
+		if (ret < 0) {
+			mlog_errno(ret);
+			goto out;
+		}
+		offset += i_size_read(inode);
+		ocfs2_inode_unlock(inode, 0);
 		break;
 	case SEEK_CUR:
 		if (offset == 0) {
_

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Mark Fasheh]

On Fri, Jan 24, 2014 at 12:47:09PM -0800, akpm at 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.

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

--
Mark Fasheh

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Andrew Morton]

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 at 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.

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

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Andrew Morton]

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.

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Mark Fasheh]

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 at 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

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Jensen]

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 at 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
>
> .
>

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Mark Fasheh]

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 at 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

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Jeff Liu]

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 at 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

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Jensen]

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 at 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

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Jensen]

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 at 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
>
> .
>

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Andrew Morton]

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() ;)

[Ocfs2-devel] [patch 09/11] ocfs2: llseek requires ocfs2 inode lock for the file in SEEK_END

[Jensen]

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.

>