| Message ID | 51A6DA6A.5060301@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/30/2013 12:49 PM, Joseph Qi wrote: > If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the > corresponding flag corrupted. So we should change it to bitwise and/or > operation. > > Signed-off-by: Joseph Qi <joseph.qi@huawei.com> Reviewed-by: Jie Liu <jeff.liu@oracle.com> > --- > fs/ocfs2/namei.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index 04ee1b5..f53471d 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir, > > fe->i_last_eb_blk = 0; > strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE); > - le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL); > + fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL); > fe->i_atime = fe->i_ctime = fe->i_mtime = > cpu_to_le64(CURRENT_TIME.tv_sec); > fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec = > @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb, > goto leave; > } > > - le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL); > + fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL); > OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR; > > /* Record which orphan dir our inode now resides > @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir, > } > > di = (struct ocfs2_dinode *)di_bh->b_data; > - le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL); > + di->i_flags &= ~cpu_to_le32(OCFS2_ORPHANED_FL); > di->i_orphaned_slot = 0; > set_nlink(inode, 1); > ocfs2_set_links_count(di, inode->i_nlink);
On Thu, 30 May 2013 12:49:46 +0800 Joseph Qi <joseph.qi@huawei.com> wrote: > If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the > corresponding flag corrupted. So we should change it to bitwise and/or > operation. > The usual question: what is the end-user impact of the bug which was just fixed? For the usual reason: so I and others can decide which kernel versions need the fix. Thanks.
Hi Andrew, Sorry for the late response because I just returned from the LinuxCon/Japan. I'm afraid that Joesph can not follow this thread up in time since today is the weekend in China. So please let me answer your question on behalf of Joesph as a quick response. On 06/01/2013 06:25 AM, Andrew Morton wrote: > On Thu, 30 May 2013 12:49:46 +0800 Joseph Qi <joseph.qi@huawei.com> wrote: > >> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the >> corresponding flag corrupted. So we should change it to bitwise and/or >> operation. >> > > The usual question: what is the end-user impact of the bug which was > just fixed? This patch can be treated as a trivial fix. Yes, i_flags is operated in bitwise context, but those assignments are only happened at the time of initializing the corresponding OCFS2 private on-disk inode info. Hence there is no impact from the end-user's perspective. However, it's better to replace those three assignments with bitwise operations since they essentially should be. In this way, we can immediately know that the business performed on i_flags are in bitwise rather than increasing or decreasing a count value. > For the usual reason: so I and others can decide which kernel versions > need the fix. The current fix comments looks too horrible than it would be. Can it make your life easier if I repost this patch with the revised descriptions if Joseph agrees to me? Thanks, -Jeff
Hi Jeff & Andrew, We found this bug when we do analysis for a read-only filesystem bug that is directly caused by dinode flags mismatch. Of course Jeff you could fix the comments and make it comprehensible. On 2013/6/1 22:34, Jeff Liu wrote: > Hi Andrew, > > Sorry for the late response because I just returned from the LinuxCon/Japan. > > I'm afraid that Joesph can not follow this thread up in time since today is the > weekend in China. So please let me answer your question on behalf of Joesph > as a quick response. > > On 06/01/2013 06:25 AM, Andrew Morton wrote: > >> On Thu, 30 May 2013 12:49:46 +0800 Joseph Qi <joseph.qi@huawei.com> wrote: >> >>> If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the >>> corresponding flag corrupted. So we should change it to bitwise and/or >>> operation. >>> >> >> The usual question: what is the end-user impact of the bug which was >> just fixed? > > This patch can be treated as a trivial fix. Yes, i_flags is operated in > bitwise context, but those assignments are only happened at the time of > initializing the corresponding OCFS2 private on-disk inode info. Hence > there is no impact from the end-user's perspective. > > However, it's better to replace those three assignments with bitwise operations > since they essentially should be. In this way, we can immediately know that the > business performed on i_flags are in bitwise rather than increasing or decreasing > a count value. > >> For the usual reason: so I and others can decide which kernel versions >> need the fix. > > The current fix comments looks too horrible than it would be. > Can it make your life easier if I repost this patch with the revised > descriptions if Joseph agrees to me? > > > Thanks, > -Jeff > > . >
diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c index 04ee1b5..f53471d 100644 --- a/fs/ocfs2/namei.c +++ b/fs/ocfs2/namei.c @@ -522,7 +522,7 @@ static int __ocfs2_mknod_locked(struct inode *dir, fe->i_last_eb_blk = 0; strcpy(fe->i_signature, OCFS2_INODE_SIGNATURE); - le32_add_cpu(&fe->i_flags, OCFS2_VALID_FL); + fe->i_flags |= cpu_to_le32(OCFS2_VALID_FL); fe->i_atime = fe->i_ctime = fe->i_mtime = cpu_to_le64(CURRENT_TIME.tv_sec); fe->i_mtime_nsec = fe->i_ctime_nsec = fe->i_atime_nsec = @@ -2044,7 +2044,7 @@ static int ocfs2_orphan_add(struct ocfs2_super *osb, goto leave; } - le32_add_cpu(&fe->i_flags, OCFS2_ORPHANED_FL); + fe->i_flags |= cpu_to_le32(OCFS2_ORPHANED_FL); OCFS2_I(inode)->ip_flags &= ~OCFS2_INODE_SKIP_ORPHAN_DIR; /* Record which orphan dir our inode now resides @@ -2434,7 +2434,7 @@ int ocfs2_mv_orphaned_inode_to_new(struct inode *dir, } di = (struct ocfs2_dinode *)di_bh->b_data; - le32_add_cpu(&di->i_flags, -OCFS2_ORPHANED_FL); + di->i_flags &= ~cpu_to_le32(OCFS2_ORPHANED_FL); di->i_orphaned_slot = 0; set_nlink(inode, 1); ocfs2_set_links_count(di, inode->i_nlink);
If we use le32_add_cpu to set ocfs2_dinode i_flags, it may lead to the corresponding flag corrupted. So we should change it to bitwise and/or operation. Signed-off-by: Joseph Qi <joseph.qi@huawei.com> --- fs/ocfs2/namei.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)