ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.
PATCH 1/1 ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.
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.
--- file.c | 7 +++++++ 1 file changed, 7 insertions(+)
file.c +0 -0
@@ -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:Signature · 1 line
1.7.9.7
Quoted 7 lines · 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.
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.
Quoted 11 lines
Signed-off-by jensen <[email protected]>--- file.c | 7 +++++++ 1 file changed, 7 insertions(+)file.c +0
-0
file.c static loff_t ocfs2_file_llseek(struct file · line 2630
Quoted 5 lines
file.c +1
-0@@ -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 ...... */
file.c static loff_t ocfs2_file_llseek(struct file · line 2637
Quoted 7 lines
file.c +6
-0+ 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?
Quoted 2 lines
file.c +0
-0break ;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
Quoted 2 lines · Jeff Liu
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).
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ 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
PATCH 1/1 ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.
In reply to Jeff LiuQuoted 10 lines · Jeff Liu
Quoted 7 lines · 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.
Quoted 4 lines
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.
Quoted 29 lines
Quoted 11 lines
Signed-off-by jensen <[email protected]>--- file.c | 7 +++++++ 1 file changed, 7 insertions(+)file.c +0
-0file.c static loff_t ocfs2_file_llseek(struct file · line 2630
Quoted 5 lines
file.c +1
-0@@ -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 ...... */file.c static loff_t ocfs2_file_llseek(struct file · line 2637
Quoted 7 lines
file.c +6
-0+ ret =ocfs2_inode_lock (inode,NULL ,0 ); +if (ret <0 ) { +mlog_errno (ret); + goto out; + } +ocfs2_inode_unlock (inode,0 ); offset += inode->i_size;
Quoted 1 line
Why not protect the offset adjustment insides ocfs2 inode locks?
yes. it need to protect the offset adjustment.
Quoted 7 lines
Quoted 3 lines
file.c +0
-0break ;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.txtfor detail if so.
Thanks you. I will see the document .
Quoted 4 lines
-Jeff
PATCH 1/1 ocfs2: llseek need to inode cluster lock and unlock for update the inode size in SEEK_END.
In reply to Mark FashehQuoted 18 lines · Mark Fasheh
Quoted 2 lines · Jeff Liu
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).
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ 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?
Quoted 7 lines
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_sizeand 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.
Quoted 6 lines
--Mark--
Mark Fasheh