[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[shencanquan]

We found that llseek has a bug when in SEEK_END.  it need to add the 
inode lock and unlock.
This bug can be reproduce the following scenario:
On one nodeA, open the file and then write some data to file and close 
the file .
On another nodeB , open the file and llseek the end of file . the 
position of file is old.

Signed-off-by: jensen <shencanquan@huawei.com>
---
  file.c |    7 +++++++
  1 file changed, 7 insertions(+)

diff --git a/file.c b/file.c
index ff54014..4ee7c80 100644
--- a/file.c
+++ b/file.c
@@ -2626,6 +2626,13 @@ static loff_t ocfs2_file_llseek(struct file 
*file, loff_t offset, int whence)
      case SEEK_SET:
          break;
      case SEEK_END:
+        /*need to inode lock and unlock for update the inode size.*/
+        ret = ocfs2_inode_lock(inode, NULL, 0);
+        if (ret < 0) {
+            mlog_errno(ret);
+            goto out;
+        }
+        ocfs2_inode_unlock(inode, 0);
          offset += inode->i_size;
          break;
      case SEEK_CUR:
-- 
1.7.9.7

[PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[shencanquan]

We found that llseek has a bug when in SEEK_END.  it need to add the 
inode lock and unlock.
This bug can be reproduce the following scenario:
On one nodeA, open the file and then write some data to file and close 
the file .
On another nodeB , open the file and llseek the end of file . the 
position of file is old.

Signed-off-by: jensen <shencanquan@huawei.com>
---
  file.c |    7 +++++++
  1 file changed, 7 insertions(+)

diff --git a/file.c b/file.c
index ff54014..4ee7c80 100644
--- a/file.c
+++ b/file.c
@@ -2626,6 +2626,13 @@ static loff_t ocfs2_file_llseek(struct file 
*file, loff_t offset, int whence)
      case SEEK_SET:
          break;
      case SEEK_END:
+        /*need to inode lock and unlock for update the inode size.*/
+        ret = ocfs2_inode_lock(inode, NULL, 0);
+        if (ret < 0) {
+            mlog_errno(ret);
+            goto out;
+        }
+        ocfs2_inode_unlock(inode, 0);
          offset += inode->i_size;
          break;
      case SEEK_CUR:
-- 
1.7.9.7

[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[Jeff Liu]

On 06/17/2013 10:48 PM, shencanquan wrote:

> We found that llseek has a bug when in SEEK_END.  it need to add the 
> inode lock and unlock.
> This bug can be reproduce the following scenario:
> On one nodeA, open the file and then write some data to file and close 
> the file .
> On another nodeB , open the file and llseek the end of file . the 
> position of file is old.

Can you please wrap these around 72 columns or so in the future?
Also, that would be appreciated if you can supply more detailed
info to reflect the results before/after applying this patch.

> 
> Signed-off-by: jensen <shencanquan@huawei.com>
> ---
>   file.c |    7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/file.c b/file.c
> index ff54014..4ee7c80 100644
> --- a/file.c
> +++ b/file.c
> @@ -2626,6 +2626,13 @@ static loff_t ocfs2_file_llseek(struct file 
> *file, loff_t offset, int whence)
>       case SEEK_SET:
>           break;
>       case SEEK_END:
> +        /*need to inode lock and unlock for update the inode size.*/

Please add spaces between comments and annotation, i.e,
	   /*  Need to ...... */

> +        ret = ocfs2_inode_lock(inode, NULL, 0);
> +        if (ret < 0) {
> +            mlog_errno(ret);
> +            goto out;
> +        }
> +        ocfs2_inode_unlock(inode, 0);
>           offset += inode->i_size;

Why not protect the offset adjustment insides ocfs2 inode locks?

>           break;
>       case SEEK_CUR:

Looks your email client has not configured properly for posting patches
because all those changes are not using tab for code indentation, please
refer to Documentation/email-clients.txt for detail if so.

-Jeff

Re: [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[Jeff Liu]

On 06/17/2013 10:48 PM, shencanquan wrote:

> We found that llseek has a bug when in SEEK_END.  it need to add the 
> inode lock and unlock.
> This bug can be reproduce the following scenario:
> On one nodeA, open the file and then write some data to file and close 
> the file .
> On another nodeB , open the file and llseek the end of file . the 
> position of file is old.

Can you please wrap these around 72 columns or so in the future?
Also, that would be appreciated if you can supply more detailed
info to reflect the results before/after applying this patch.

> 
> Signed-off-by: jensen <shencanquan@huawei.com>
> ---
>   file.c |    7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/file.c b/file.c
> index ff54014..4ee7c80 100644
> --- a/file.c
> +++ b/file.c
> @@ -2626,6 +2626,13 @@ static loff_t ocfs2_file_llseek(struct file 
> *file, loff_t offset, int whence)
>       case SEEK_SET:
>           break;
>       case SEEK_END:
> +        /*need to inode lock and unlock for update the inode size.*/

Please add spaces between comments and annotation, i.e,
	   /*  Need to ...... */

> +        ret = ocfs2_inode_lock(inode, NULL, 0);
> +        if (ret < 0) {
> +            mlog_errno(ret);
> +            goto out;
> +        }
> +        ocfs2_inode_unlock(inode, 0);
>           offset += inode->i_size;

Why not protect the offset adjustment insides ocfs2 inode locks?

>           break;
>       case SEEK_CUR:

Looks your email client has not configured properly for posting patches
because all those changes are not using tab for code indentation, please
refer to Documentation/email-clients.txt for detail if so.

-Jeff

[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[Mark Fasheh]

On Mon, Jun 17, 2013 at 11:17:49PM +0800, Jeff Liu wrote:
> Please add spaces between comments and annotation, i.e,
> 	   /*  Need to ...... */

yes, please describe while we're taking the lock here (describing the race
you're talking about would be fine).


> > +        ret = ocfs2_inode_lock(inode, NULL, 0);
> > +        if (ret < 0) {
> > +            mlog_errno(ret);
> > +            goto out;
> > +        }
> > +        ocfs2_inode_unlock(inode, 0);
> >           offset += inode->i_size;
> 
> Why not protect the offset adjustment insides ocfs2 inode locks?

If we're going through the trouble of locking we *need* to put the offset
calculation inside the lock otherwise we can still get stale i_size and
basically haven't fixed anything.


Btw, do you feel that this could impact performance for other workloads that
ocfs2 usually does?
	--Mark

--
Mark Fasheh

Re: [Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[Mark Fasheh]

On Mon, Jun 17, 2013 at 11:17:49PM +0800, Jeff Liu wrote:
> Please add spaces between comments and annotation, i.e,
> 	   /*  Need to ...... */

yes, please describe while we're taking the lock here (describing the race
you're talking about would be fine).


> > +        ret = ocfs2_inode_lock(inode, NULL, 0);
> > +        if (ret < 0) {
> > +            mlog_errno(ret);
> > +            goto out;
> > +        }
> > +        ocfs2_inode_unlock(inode, 0);
> >           offset += inode->i_size;
> 
> Why not protect the offset adjustment insides ocfs2 inode locks?

If we're going through the trouble of locking we *need* to put the offset
calculation inside the lock otherwise we can still get stale i_size and
basically haven't fixed anything.


Btw, do you feel that this could impact performance for other workloads that
ocfs2 usually does?
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[shencanquan]

On 2013/6/18 1:02, Mark Fasheh wrote:

> On Mon, Jun 17, 2013 at 11:17:49PM +0800, Jeff Liu wrote:
>> Please add spaces between comments and annotation, i.e,
>> 	   /*  Need to ...... */
> 
> yes, please describe while we're taking the lock here (describing the race
> you're talking about would be fine).
> 
> 
>>> +        ret = ocfs2_inode_lock(inode, NULL, 0);
>>> +        if (ret < 0) {
>>> +            mlog_errno(ret);
>>> +            goto out;
>>> +        }
>>> +        ocfs2_inode_unlock(inode, 0);
>>>           offset += inode->i_size;
>>
>> Why not protect the offset adjustment insides ocfs2 inode locks?
> 
> If we're going through the trouble of locking we *need* to put the offset
> calculation inside the lock otherwise we can still get stale i_size and
> basically haven't fixed anything.
> 
> 
> Btw, do you feel that this could impact performance for other workloads that
> ocfs2 usually does?


I think it maybe impact performance. because it use the cluster lock and
it maybe flush the inode cache to disk and read the inode from disk. but
I think the correct function of llseek is more important.


> 	--Mark
> 
> --
> Mark Fasheh
> 
> 

[Ocfs2-devel] [PATCH] ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.

[shencanquan]

On 2013/6/17 23:17, Jeff Liu wrote:

> On 06/17/2013 10:48 PM, shencanquan wrote:
> 
>> We found that llseek has a bug when in SEEK_END.  it need to add the 
>> inode lock and unlock.
>> This bug can be reproduce the following scenario:
>> On one nodeA, open the file and then write some data to file and close 
>> the file .
>> On another nodeB , open the file and llseek the end of file . the 
>> position of file is old.
> 
> Can you please wrap these around 72 columns or so in the future?
> Also, that would be appreciated if you can supply more detailed
> info to reflect the results before/after applying this patch.
> 

 Thanks for your comments.

>>
>> Signed-off-by: jensen <shencanquan@huawei.com>
>> ---
>>   file.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/file.c b/file.c
>> index ff54014..4ee7c80 100644
>> --- a/file.c
>> +++ b/file.c
>> @@ -2626,6 +2626,13 @@ static loff_t ocfs2_file_llseek(struct file 
>> *file, loff_t offset, int whence)
>>       case SEEK_SET:
>>           break;
>>       case SEEK_END:
>> +        /*need to inode lock and unlock for update the inode size.*/
> 
> Please add spaces between comments and annotation, i.e,

> 	   /*  Need to ...... */
> 
>> +        ret = ocfs2_inode_lock(inode, NULL, 0);
>> +        if (ret < 0) {
>> +            mlog_errno(ret);
>> +            goto out;
>> +        }
>> +        ocfs2_inode_unlock(inode, 0);
>>           offset += inode->i_size;
> 
> Why not protect the offset adjustment insides ocfs2 inode locks?

yes. it need to protect the offset adjustment.

> 
>>           break;
>>       case SEEK_CUR:
> 
> Looks your email client has not configured properly for posting patches
> because all those changes are not using tab for code indentation, please
> refer to Documentation/email-clients.txt for detail if so.

Thanks you. I will see the document .

> 
> -Jeff
> 
>