ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
PATCH 1/1 ocfs2: llseek requires to 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.
if it not . after call llseek in SEEK_END. the position is old.
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 apply this patch. the three step result is 15k.
--- fs/ocfs2/file.c | 9 +++++++++ 1 file changed, 9 insertions(+)
fs/ocfs2/file.c +9 -0
@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END : +/* 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 += inode->i_size; +ocfs2_inode_unlock (inode,0 );break ;case SEEK_CUR :if (offset ==0 ) {
Signature · 1 line
1.7.9.7
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 31 lines · On Thu, 20 Jun 2013 16:23:59 +0800 shencanquan <[email protected]> wrote:
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
...
fs/ocfs2/file.c +1
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END : +/* SEEK_END requires the OCFS2 inode lock for the file
fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2639
Quoted 10 lines
fs/ocfs2/file.c +8
-0+* because it references the file's size. +*/ + ret =ocfs2_inode_lock (inode,NULL ,0 ); +if (ret <0 ) { +mlog_errno (ret); + goto out; + } offset += inode->i_size; +ocfs2_inode_unlock (inode,0 );break ;
I don't understand this. The lock for inode->i_size is inode->i_mutex, and we're already holding i_mutex here. The current mainline code looks correct.
My guess is that there is some other code path which is modifying inode->i_size without holding inode->i_mutex, and while holding ocfs2_inode_lock(). If so, that code is surely wrong - it should hold i_mutex while modifying i_size.
Also, safely reading i_size should be performed via i_size_read(), and modifications to i_size should use i_size_write().
And all this is only really applicable to 32-bit CPUs, which you probably aren't using.
So.... please let's take a second look at this.
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 48 lines · Andrew Morton
Quoted 31 lines · On Thu, 20 Jun 2013 16:23:59 +0800 shencanquan <[email protected]> wrote:
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
...
fs/ocfs2/file.c +1
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END : +/* SEEK_END requires the OCFS2 inode lock for the file fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2639
Quoted 10 lines
fs/ocfs2/file.c +8
-0+* because it references the file's size. +*/ + ret =ocfs2_inode_lock (inode,NULL ,0 ); +if (ret <0 ) { +mlog_errno (ret); + goto out; + } offset += inode->i_size; +ocfs2_inode_unlock (inode,0 );break ;I don't understand this. The lock for
inode->i_sizeisinode->i_mutex, and we're already holdingi_mutexhere. The current mainline code looks correct.
Quoted 4 lines
My guess is that there is some other code path which is modifying
inode->i_sizewithout holdinginode->i_mutex, and while holdingocfs2_inode_lock(). If so, that code is surely wrong - it should holdi_mutexwhile modifyingi_size.
inode->i_mutex lock only protect the inode size on the same machine. because ocfs2 is cluster file system. so other machine maybe append the file. and modify the file size.
and so before get the inode size, it need cluster lock, when other node unlock cluster lock it will write the inode size to disk. and this machine acquire the lock it will read the inode size from disk.
Quoted 4 lines
Also, safely reading
i_sizeshould be performed viai_size_read(), and modifications toi_sizeshould usei_size_write().
ok, thanks for your comments, I will modify this and resent the patch.
Quoted 2 lines
And all this is only really applicable to 32-bit CPUs, which you probably aren't using.
I don't understand this.
Quoted 5 lines
So.... please let's take a second look at this.
.
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 10 lines · On Thu, 27 Jun 2013 09:19:52 +0800 shencanquan <[email protected]> wrote:
Quoted 5 lines · Andrew Morton
My guess is that there is some other code path which is modifying
inode->i_sizewithout holdinginode->i_mutex, and while holdingocfs2_inode_lock(). If so, that code is surely wrong - it should holdi_mutexwhile modifyingi_size.
inode->i_mutexlock only protect the inode size on the same machine.
ah ;)
Quoted 4 lines
Quoted 2 lines
And all this is only really applicable to 32-bit CPUs, which you probably aren't using.
I don't understand this.
The i_size_read/i_size_write infrastructure is designed to efficiently handle the situation where a 32-bit machine reads a 64-bit number which might be undergoing modification on another CPU. We don't want the reading CPU to see an invalid number when the writing CPU is in the middle of modifying the two 32-bit words.
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 18 lines · Andrew Morton
Quoted 10 lines · On Thu, 27 Jun 2013 09:19:52 +0800 shencanquan <[email protected]> wrote:
Quoted 5 lines · Andrew Morton
My guess is that there is some other code path which is modifying
inode->i_sizewithout holdinginode->i_mutex, and while holdingocfs2_inode_lock(). If so, that code is surely wrong - it should holdi_mutexwhile modifyingi_size.
inode->i_mutexlock only protect the inode size on the same machine.ah ;)
Quoted 3 lines
Quoted 2 lines
And all this is only really applicable to 32-bit CPUs, which you probably aren't using.
Quoted 10 lines
Quoted 1 line
I don't understand this.
The i_size_read/i_size_write infrastructure is designed to efficiently handle the situation where a 32-bit machine reads a 64-bit number which might be undergoing modification on another CPU. We don't want the reading CPU to see an invalid number when the writing CPU is in the middle of modifying the two 32-bit words.
ok. thanks.
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
AFAIR, this behavior has been there since day 1 and changing it will impact performance negatively. I would recommend against making this change for one app.
Quoted 32 lines · shencanquan
Quoted 1 line · Andrew Morton
On Thu, 27 Jun 2013 09:19:52 +0800 shencanquan <[email protected]>
Quoted 17 lines · wrote:
Quoted 11 lines · Andrew Morton
Quoted 6 lines
My guess is that there is some other code path which is modifying
inode->i_sizewithout holdinginode->i_mutex, and while holdingocfs2_inode_lock(). If so, that code is surely wrong - it should holdi_mutexwhile modifyingi_size.
inode->i_mutexlock only protect the inode size on the same machine.ah ;)
Quoted 3 lines
Quoted 2 lines
And all this is only really applicable to 32-bit CPUs, which you probably aren't using.
Quoted 10 lines
Quoted 1 line
I don't understand this.
The i_size_read/i_size_write infrastructure is designed to efficiently handle the situation where a 32-bit machine reads a 64-bit number which might be undergoing modification on another CPU. We don't want the reading CPU to see an invalid number when the writing CPU is in the middle of modifying the two 32-bit words.
Quoted 8 lines
ok. thanks.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20130626/3f723c36/attachment.html
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 3 lines · Sunil Mushran
AFAIR, this behavior has been there since day 1 and changing it will impact performance negatively. I would recommend against making this change for one app.
I think it will impact the performance negatively. but I think it is very very little, because in cluster lock function, the inode size will update from dlm in lvb. and it don't update by disk (another machine write file size to disk and this machine read file size from disk to update the inode size)
if the file size update by disk. I think it will impact the performance very much.
Quoted 43 lines · On Wed, Jun 26, 2013 at 6:50 PM, shencanquan <shencanquan at huawei.com…
On 2013/6/27 9:25, Andrew Morton wrote:
> On Thu, 27 Jun 2013 09:19:52 +0800 shencanquan <shencanquan at huawei.com <mailto:[email protected]>> wrote:
>
>> On 2013/6/27 5:18, Andrew Morton wrote:
>>
>>>
>>> My guess is that there is some other code path which is modifying
>>> inode->i_size without holding inode->i_mutex, and while holding
>>> ocfs2_inode_lock(). If so, that code is surely wrong - it should hold
>>> i_mutex while modifying i_size.
>>
>>
>> inode->i_mutex lock only protect the inode size on the same machine.
>
> ah ;)
>
>>> And all this is only really applicable to 32-bit CPUs, which you
>>> probably aren't using.
>>
>> I don't understand this.
>
> The i_size_read/i_size_write infrastructure is designed to efficiently
> handle the situation where a 32-bit machine reads a 64-bit number which
> might be undergoing modification on another CPU. We don't want the
> reading CPU to see an invalid number when the writing CPU is in the
> middle of modifying the two 32-bit words.
>
>
>
ok. thanks.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com <mailto:[email protected]>
https://oss.oracle.com/mailman/listinfo/ocfs2-develPATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
In reply to Sunil MushranQuoted 3 lines · On Wed, 26 Jun 2013 20:34:19 -0700 Sunil Mushran <[email protected]>…
AFAIR, this behavior has been there since day 1 and changing it will impact performance negatively. I would recommend against making this change for one app.
Well, it's a bug fix isn't it? SEEK_END on a 15k file is presently returning 10k.
Obviously tradeoffs are involved - is there any way in which this behaviour can be fixed without serious performance damage?
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
The qs is whether this change is required for a real problem or not. If so, what is that logic
that gets tripped up by this behaviour.
Quoted 8 lines · Andrew Morton
Quoted 1 line · On Wed, 26 Jun 2013 20:34:19 -0700 Sunil Mushran <[email protected]>…
AFAIR, this behavior has been there since day 1 and changing it will
impact
Quoted 2 lines
performance negatively. I would recommend against making this change for one app.
Quoted 8 lines
Well, it's a bug fix isn't it?
SEEK_ENDon a 15k file is presently returning
10k.Obviously tradeoffs are involved - is there any way in which this behaviour can be fixed without serious performance damage?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20130627/8d8a26f6/attachment.html
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 3 lines · Sunil Mushran
The qs is whether this change is required for a real problem or not. If so, what is that logic
that gets tripped up by this behaviour.
IMHO, there have a couple of commands in Coreutils would be affected by this behavior:
- tail(1) with "-c bytes" option.
- wc(1): when counting only bytes, it will try to figure out the file size through lseek (SEEK_END - SEEK_CUR) when counting only bytes.
Other popular programs need get the file size via SEEK_END only in some corner cases. e.g, if the input file is not a regular file or the utility failed to fetch the file size via fstat(2), they will call lseek as an alternative approach, like split(1), truncate(1), head(1) with '-c bytes' option.
Generally, the user applications might fetch the file size through fstat(2) which will go through ocfs2_getattr() so that it's safe to us because we perform inode re-validation before filling up the generic attributes.
IOWs, the user application could be changed accordingly to avoid this issue. However, this most likely a workaround to some extent, and we have a bug for the SEEK_END business.
As per our current discussion, one big concern of us is regarding the performance with this fix, I'd like to consider it from another point of view, that is the user should take all the blame to themselves if they would like to run SEEK_END bounds operation on OCFS2, does it sounds make sense? :-P.
If yes, it would be better to consolidate the code comments to indicate the performance negatively and it is not advisable to fetch file size via SEEK_END on OCFS2.
Thanks,
-Jeff
Quoted 28 lines
On Thu, Jun 27, 2013 at 3:08 PM, Andrew Morton
<akpm at linux-foundation.org <mailto:[email protected]>> wrote:On Wed, 26 Jun 2013 20:34:19 -0700 Sunil Mushran <sunil.mushran at gmail.com <mailto:[email protected]>> wrote:
> AFAIR, this behavior has been there since day 1 and changing it will impact
> performance negatively. I would recommend against making this change for
> one app.Well, it's a bug fix isn't it?
SEEK_ENDon a 15k file is presently returning
10k.Obviously tradeoffs are involved - is there any way in which this behaviour can be fixed without serious performance damage?
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Joel Becker
maintainerPATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 5 lines · Jeff Liu
Quoted 3 lines · Sunil Mushran
The qs is whether this change is required for a real problem or not. If so, what is that logic
that gets tripped up by this behaviour.
This is a straight correctness fix. We take the hit and apply it. I'll check the actual patch on that email.
Joel
Quoted 67 lines
IMHO, there have a couple of commands in Coreutils would be affected by this behavior:
- tail(1) with "-c bytes" option.
- wc(1): when counting only bytes, it will try to figure out the file size through lseek (SEEK_END-SEEK_CUR) when counting only bytes.Other popular programs need get the file size via
SEEK_ENDonly in some corner cases. e.g, if the input file is not a regular file or the utility failed to fetch the file size via fstat(2), they will call lseek as an alternative approach, like split(1), truncate(1), head(1) with '-c bytes' option.Generally, the user applications might fetch the file size through fstat(2) which will go through
ocfs2_getattr()so that it's safe to us because we perform inode re-validation before filling up the generic attributes.IOWs, the user application could be changed accordingly to avoid this issue. However, this most likely a workaround to some extent, and we have a bug for the
SEEK_ENDbusiness.As per our current discussion, one big concern of us is regarding the performance with this fix, I'd like to consider it from another point of view, that is the user should take all the blame to themselves if they would like to run
SEEK_ENDbounds operation on OCFS2, does it sounds make sense? :-P.
If yes, it would be better to consolidate the code comments to indicate the performance negatively and it is not advisable to fetch file size viaSEEK_ENDon OCFS2.Thanks,
-JeffQuoted 24 lines
On Thu, Jun 27, 2013 at 3:08 PM, Andrew Morton
<akpm at linux-foundation.org <mailto:[email protected]>> wrote:On Wed, 26 Jun 2013 20:34:19 -0700 Sunil Mushran <sunil.mushran at gmail.com <mailto:[email protected]>> wrote:
> AFAIR, this behavior has been there since day 1 and changing it will impact
> performance negatively. I would recommend against making this change for
> one app.Well, it's a bug fix isn't it?
SEEK_ENDon a 15k file is presently returning
10k.Obviously tradeoffs are involved - is there any way in which this behaviour can be fixed without serious performance damage?
Quoted 4 lines
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Joel Becker
maintainerPATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
In reply to shencanquanQuoted 35 lines · shencanquan
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
Signed-off-by jensen <[email protected]>--- fs/ocfs2/file.c | 9 +++++++++ 1 file changed, 9 insertions(+)fs/ocfs2/file.c +0
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END :
fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2638
Quoted 10 lines
fs/ocfs2/file.c +9
-0+/* 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 += inode->i_size; +ocfs2_inode_unlock (inode,0 );
Why wouldn't ocfs2_rw_lock() work? Just because we dont get the LVB from it?
Joel
Quoted 11 lines
fs/ocfs2/file.c +0
-0break ;case SEEK_CUR :if (offset ==0 ) {Signature · 7 lines
1.7.9.7 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel at oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 41 lines · Joel Becker
Quoted 40 lines · shencanquan
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
Signed-off-by jensen <[email protected]>--- fs/ocfs2/file.c | 9 +++++++++ 1 file changed, 9 insertions(+)fs/ocfs2/file.c +5
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END : +/* 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 ) {
Quoted 9 lines
fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2638
Quoted 5 lines
fs/ocfs2/file.c +4
-0+mlog_errno (ret); + goto out; + } offset += inode->i_size; +ocfs2_inode_unlock (inode,0 );Why wouldn't
ocfs2_rw_lock()work? Just because we dont get the LVB from it?
Yes. we want to update the inode size from lvb.
I also think the file size maybe protected by inode lock. not rw lock.
Quoted 14 lines
Joel
Quoted 11 lines
fs/ocfs2/file.c +0
-0break ;case SEEK_CUR :if (offset ==0 ) {Signature · 7 lines
1.7.9.7 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel at oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 54 lines · Jensen
Quoted 41 lines · Joel Becker
Quoted 40 lines · shencanquan
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
Signed-off-by jensen <[email protected]>--- fs/ocfs2/file.c | 9 +++++++++ 1 file changed, 9 insertions(+)fs/ocfs2/file.c +5
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END : +/* 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 ) {Quoted 9 lines
fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2638
Quoted 5 lines
fs/ocfs2/file.c +4
-0+mlog_errno (ret); + goto out; + } offset += inode->i_size; +ocfs2_inode_unlock (inode,0 );Why wouldn't
ocfs2_rw_lock()work? Just because we dont get the LVB from it?
Quoted 3 lines
Yes. we want to update the inode size from lvb.
I also think the file size maybe protected by inode lock. not rw lock.
Correct, if you want to get the most up to date i_size you'll have to be holding the meta (inode) lock.
--Mark
--
Mark Fasheh
Joel Becker
maintainerPATCH 1/1 ocfs2: llseek requires to ocfs2 inode lock for the file in SEEK_END
Quoted 55 lines · Mark Fasheh
Quoted 54 lines · Jensen
Quoted 50 lines · Joel Becker
Quoted 35 lines · shencanquan
llseek requires ocfs2 inode lock for updating the file size in
SEEK_END. because the file size maybe update on another node.
if it not . after call llseek inSEEK_END. the position is old.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 fileAon 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 apply this patch. the three step result is 15k.
Signed-off-by jensen <[email protected]>--- fs/ocfs2/file.c | 9 +++++++++ 1 file changed, 9 insertions(+)fs/ocfs2/file.c +0
-0@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)case SEEK_SET :break ;case SEEK_END :fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2638
Quoted 10 lines
fs/ocfs2/file.c +9
-0+/* 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 += inode->i_size; +ocfs2_inode_unlock (inode,0 );Why wouldn't
ocfs2_rw_lock()work? Just because we dont get the LVB from it?
Quoted 6 lines
Quoted 3 lines
Yes. we want to update the inode size from lvb.
I also think the file size maybe protected by inode lock. not rw lock.
Correct, if you want to get the most up to date
i_sizeyou'll have to be holding the meta (inode) lock.
Ok, then:
Quoted 9 lines
--Mark--
Mark Fasheh_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Signature · 5 lines
"Vote early and vote often."
- Al Capone
http://www.jlbec.org/
jlbec at evilplan.org