| Message ID | 51BF21D6.9080008@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 > >
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 > >
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;
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(+) case SEEK_CUR: