| Message ID | 20140206155029.b7275670913c34ef6bc9a530@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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 > > . >
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
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
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
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 > > . >
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() ;)
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. >
--- 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; }