| Message ID | 20150419054303.GA8548@devel.8.8.4.4 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Joseph. Can you review this patch again, actually you had reviewed by 'https://lkml.org/lkml/2015/2/27/655'. Thanks. regards, Daeseok Youn 2015-04-19 14:43 GMT+09:00 Daeseok Youn <daeseok.youn@gmail.com>: > The use of 'status' in __ocfs2_add_entry() can return wrong > value. Some functions' return value in __ocfs2_add_entry(), > i.e ocfs2_journal_access_di() is saved to 'status'. > But 'status' is not used in 'bail' label for returning result > of __ocfs2_add_entry(). > > So use retval instead of status. > > Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com> > --- > This patch was came from 'https://lkml.org/lkml/2015/2/27/655' > This patch was needed to test but I didn't have any environment > for testing ocfs2 filesystem. But I have one, now. > (I'm too busy to make this enviroment. And qemu for this fs is difficult > for me. :-(, sorry for that) > > Briefly how to set my environment for testing this fs with qemu. > 1. Getting and building linux kernel with linux-next branch for x86_64 > qemu. > And also options of ocfs2 related are enabled(built-in) > 2. Makes own root file system with 'buildroot' project. > 3. Getting and building ocfs2-tools. > Then binaries after building this tool are moved my rootfs. > 4. Makes dummy disk image(5G) which will be formatted in qemu. > 5. Booting qemu with rootfs image and dummy disk image. > 6. mkfs.ocfs2 --fs-feature=local <device> > this maybe possilbe to mount standalone mode. > 7. tunefs.ocfs2 --fs-features=indexed-dirs,noinline-data <device> > 8. make a cluster and one node > use o2cb_ctl tool. > 9. o2cb service load and initialize > # /etc/init.d/o2cb load && /etc/init.d/o2cb configure > # /etc/init.d/o2cb online > 10. mount ocfs2 > # mount.ocfs2 <device> <some directory> > > And use GDB for debugging my patch path. > Connect gdb with qemu and add breakpoint in __ocfs2_add_entry() of > fs/ocfs2/dir.c > > And test my patch. > # cd <some directory mounted with ocfs2> > # mkdir <specific directory> > > This how-to is not written all my work, just briefly I said. > > fs/ocfs2/dir.c | 20 ++++++++++---------- > 1 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index 990e8f7..a9513ff 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -1607,7 +1607,7 @@ int __ocfs2_add_entry(handle_t *handle, > struct ocfs2_dir_entry *de, *de1; > struct ocfs2_dinode *di = (struct ocfs2_dinode > *)parent_fe_bh->b_data; > struct super_block *sb = dir->i_sb; > - int retval, status; > + int retval; > unsigned int size = sb->s_blocksize; > struct buffer_head *insert_bh = lookup->dl_leaf_bh; > char *data_start = insert_bh->b_data; > @@ -1685,25 +1685,25 @@ int __ocfs2_add_entry(handle_t *handle, > } > > if (insert_bh == parent_fe_bh) > - status = ocfs2_journal_access_di(handle, > + retval = ocfs2_journal_access_di(handle, > > INODE_CACHE(dir), > insert_bh, > > OCFS2_JOURNAL_ACCESS_WRITE); > else { > - status = ocfs2_journal_access_db(handle, > + retval = ocfs2_journal_access_db(handle, > > INODE_CACHE(dir), > insert_bh, > OCFS2_JOURNAL_ACCESS_WRITE); > > - if (ocfs2_dir_indexed(dir)) { > - status = ocfs2_dx_dir_insert(dir, > + if (!retval && ocfs2_dir_indexed(dir)) > + retval = ocfs2_dx_dir_insert(dir, > handle, > lookup); > - if (status) { > - mlog_errno(status); > - goto bail; > - } > - } > + } > + > + if (retval) { > + mlog_errno(retval); > + goto bail; > } > > /* By now the buffer is marked for journaling */ > -- > 1.7.1 > >
On 2015/4/19 13:43, Daeseok Youn wrote: > The use of 'status' in __ocfs2_add_entry() can return wrong > value. Some functions' return value in __ocfs2_add_entry(), > i.e ocfs2_journal_access_di() is saved to 'status'. > But 'status' is not used in 'bail' label for returning result > of __ocfs2_add_entry(). > > So use retval instead of status. > > Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com> Reviewed-by: Joseph Qi <joseph.qi@huawei.com> > --- > This patch was came from 'https://lkml.org/lkml/2015/2/27/655' > This patch was needed to test but I didn't have any environment > for testing ocfs2 filesystem. But I have one, now. > (I'm too busy to make this enviroment. And qemu for this fs is difficult > for me. :-(, sorry for that) > > Briefly how to set my environment for testing this fs with qemu. > 1. Getting and building linux kernel with linux-next branch for x86_64 qemu. > And also options of ocfs2 related are enabled(built-in) > 2. Makes own root file system with 'buildroot' project. > 3. Getting and building ocfs2-tools. > Then binaries after building this tool are moved my rootfs. > 4. Makes dummy disk image(5G) which will be formatted in qemu. > 5. Booting qemu with rootfs image and dummy disk image. > 6. mkfs.ocfs2 --fs-feature=local <device> > this maybe possilbe to mount standalone mode. > 7. tunefs.ocfs2 --fs-features=indexed-dirs,noinline-data <device> > 8. make a cluster and one node > use o2cb_ctl tool. > 9. o2cb service load and initialize > # /etc/init.d/o2cb load && /etc/init.d/o2cb configure > # /etc/init.d/o2cb online > 10. mount ocfs2 > # mount.ocfs2 <device> <some directory> > > And use GDB for debugging my patch path. > Connect gdb with qemu and add breakpoint in __ocfs2_add_entry() of fs/ocfs2/dir.c > > And test my patch. > # cd <some directory mounted with ocfs2> > # mkdir <specific directory> > > This how-to is not written all my work, just briefly I said. > > fs/ocfs2/dir.c | 20 ++++++++++---------- > 1 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index 990e8f7..a9513ff 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -1607,7 +1607,7 @@ int __ocfs2_add_entry(handle_t *handle, > struct ocfs2_dir_entry *de, *de1; > struct ocfs2_dinode *di = (struct ocfs2_dinode *)parent_fe_bh->b_data; > struct super_block *sb = dir->i_sb; > - int retval, status; > + int retval; > unsigned int size = sb->s_blocksize; > struct buffer_head *insert_bh = lookup->dl_leaf_bh; > char *data_start = insert_bh->b_data; > @@ -1685,25 +1685,25 @@ int __ocfs2_add_entry(handle_t *handle, > } > > if (insert_bh == parent_fe_bh) > - status = ocfs2_journal_access_di(handle, > + retval = ocfs2_journal_access_di(handle, > INODE_CACHE(dir), > insert_bh, > OCFS2_JOURNAL_ACCESS_WRITE); > else { > - status = ocfs2_journal_access_db(handle, > + retval = ocfs2_journal_access_db(handle, > INODE_CACHE(dir), > insert_bh, > OCFS2_JOURNAL_ACCESS_WRITE); > > - if (ocfs2_dir_indexed(dir)) { > - status = ocfs2_dx_dir_insert(dir, > + if (!retval && ocfs2_dir_indexed(dir)) > + retval = ocfs2_dx_dir_insert(dir, > handle, > lookup); > - if (status) { > - mlog_errno(status); > - goto bail; > - } > - } > + } > + > + if (retval) { > + mlog_errno(retval); > + goto bail; > } > > /* By now the buffer is marked for journaling */ >
Hi, Joseph. 2015-04-23 9:49 GMT+09:00 Joseph Qi <joseph.qi@huawei.com>: > On 2015/4/19 13:43, Daeseok Youn wrote: >> The use of 'status' in __ocfs2_add_entry() can return wrong >> value. Some functions' return value in __ocfs2_add_entry(), >> i.e ocfs2_journal_access_di() is saved to 'status'. >> But 'status' is not used in 'bail' label for returning result >> of __ocfs2_add_entry(). >> >> So use retval instead of status. >> >> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com> > > Reviewed-by: Joseph Qi <joseph.qi@huawei.com> Thanks for review. regards, Daeseok Youn. >> --- >> This patch was came from 'https://lkml.org/lkml/2015/2/27/655' >> This patch was needed to test but I didn't have any environment >> for testing ocfs2 filesystem. But I have one, now. >> (I'm too busy to make this enviroment. And qemu for this fs is difficult >> for me. :-(, sorry for that) >> >> Briefly how to set my environment for testing this fs with qemu. >> 1. Getting and building linux kernel with linux-next branch for x86_64 qemu. >> And also options of ocfs2 related are enabled(built-in) >> 2. Makes own root file system with 'buildroot' project. >> 3. Getting and building ocfs2-tools. >> Then binaries after building this tool are moved my rootfs. >> 4. Makes dummy disk image(5G) which will be formatted in qemu. >> 5. Booting qemu with rootfs image and dummy disk image. >> 6. mkfs.ocfs2 --fs-feature=local <device> >> this maybe possilbe to mount standalone mode. >> 7. tunefs.ocfs2 --fs-features=indexed-dirs,noinline-data <device> >> 8. make a cluster and one node >> use o2cb_ctl tool. >> 9. o2cb service load and initialize >> # /etc/init.d/o2cb load && /etc/init.d/o2cb configure >> # /etc/init.d/o2cb online >> 10. mount ocfs2 >> # mount.ocfs2 <device> <some directory> >> >> And use GDB for debugging my patch path. >> Connect gdb with qemu and add breakpoint in __ocfs2_add_entry() of fs/ocfs2/dir.c >> >> And test my patch. >> # cd <some directory mounted with ocfs2> >> # mkdir <specific directory> >> >> This how-to is not written all my work, just briefly I said. >> >> fs/ocfs2/dir.c | 20 ++++++++++---------- >> 1 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >> index 990e8f7..a9513ff 100644 >> --- a/fs/ocfs2/dir.c >> +++ b/fs/ocfs2/dir.c >> @@ -1607,7 +1607,7 @@ int __ocfs2_add_entry(handle_t *handle, >> struct ocfs2_dir_entry *de, *de1; >> struct ocfs2_dinode *di = (struct ocfs2_dinode *)parent_fe_bh->b_data; >> struct super_block *sb = dir->i_sb; >> - int retval, status; >> + int retval; >> unsigned int size = sb->s_blocksize; >> struct buffer_head *insert_bh = lookup->dl_leaf_bh; >> char *data_start = insert_bh->b_data; >> @@ -1685,25 +1685,25 @@ int __ocfs2_add_entry(handle_t *handle, >> } >> >> if (insert_bh == parent_fe_bh) >> - status = ocfs2_journal_access_di(handle, >> + retval = ocfs2_journal_access_di(handle, >> INODE_CACHE(dir), >> insert_bh, >> OCFS2_JOURNAL_ACCESS_WRITE); >> else { >> - status = ocfs2_journal_access_db(handle, >> + retval = ocfs2_journal_access_db(handle, >> INODE_CACHE(dir), >> insert_bh, >> OCFS2_JOURNAL_ACCESS_WRITE); >> >> - if (ocfs2_dir_indexed(dir)) { >> - status = ocfs2_dx_dir_insert(dir, >> + if (!retval && ocfs2_dir_indexed(dir)) >> + retval = ocfs2_dx_dir_insert(dir, >> handle, >> lookup); >> - if (status) { >> - mlog_errno(status); >> - goto bail; >> - } >> - } >> + } >> + >> + if (retval) { >> + mlog_errno(retval); >> + goto bail; >> } >> >> /* By now the buffer is marked for journaling */ >> > >
Hi, Andrew. Is there problem this patch to merge? If you care about mlog_errno() cleanup, I will send another patch for that. And also if you have any reasons this patch has not been taken, please let me know. I will fix and test again. Thanks. regards, Daeseok Youn/ 2015-04-23 9:49 GMT+09:00 Joseph Qi <joseph.qi@huawei.com>: > On 2015/4/19 13:43, Daeseok Youn wrote: >> The use of 'status' in __ocfs2_add_entry() can return wrong >> value. Some functions' return value in __ocfs2_add_entry(), >> i.e ocfs2_journal_access_di() is saved to 'status'. >> But 'status' is not used in 'bail' label for returning result >> of __ocfs2_add_entry(). >> >> So use retval instead of status. >> >> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com> > > Reviewed-by: Joseph Qi <joseph.qi@huawei.com> >> --- >> This patch was came from 'https://lkml.org/lkml/2015/2/27/655' >> This patch was needed to test but I didn't have any environment >> for testing ocfs2 filesystem. But I have one, now. >> (I'm too busy to make this enviroment. And qemu for this fs is difficult >> for me. :-(, sorry for that) >> >> Briefly how to set my environment for testing this fs with qemu. >> 1. Getting and building linux kernel with linux-next branch for x86_64 qemu. >> And also options of ocfs2 related are enabled(built-in) >> 2. Makes own root file system with 'buildroot' project. >> 3. Getting and building ocfs2-tools. >> Then binaries after building this tool are moved my rootfs. >> 4. Makes dummy disk image(5G) which will be formatted in qemu. >> 5. Booting qemu with rootfs image and dummy disk image. >> 6. mkfs.ocfs2 --fs-feature=local <device> >> this maybe possilbe to mount standalone mode. >> 7. tunefs.ocfs2 --fs-features=indexed-dirs,noinline-data <device> >> 8. make a cluster and one node >> use o2cb_ctl tool. >> 9. o2cb service load and initialize >> # /etc/init.d/o2cb load && /etc/init.d/o2cb configure >> # /etc/init.d/o2cb online >> 10. mount ocfs2 >> # mount.ocfs2 <device> <some directory> >> >> And use GDB for debugging my patch path. >> Connect gdb with qemu and add breakpoint in __ocfs2_add_entry() of fs/ocfs2/dir.c >> >> And test my patch. >> # cd <some directory mounted with ocfs2> >> # mkdir <specific directory> >> >> This how-to is not written all my work, just briefly I said. >> >> fs/ocfs2/dir.c | 20 ++++++++++---------- >> 1 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c >> index 990e8f7..a9513ff 100644 >> --- a/fs/ocfs2/dir.c >> +++ b/fs/ocfs2/dir.c >> @@ -1607,7 +1607,7 @@ int __ocfs2_add_entry(handle_t *handle, >> struct ocfs2_dir_entry *de, *de1; >> struct ocfs2_dinode *di = (struct ocfs2_dinode *)parent_fe_bh->b_data; >> struct super_block *sb = dir->i_sb; >> - int retval, status; >> + int retval; >> unsigned int size = sb->s_blocksize; >> struct buffer_head *insert_bh = lookup->dl_leaf_bh; >> char *data_start = insert_bh->b_data; >> @@ -1685,25 +1685,25 @@ int __ocfs2_add_entry(handle_t *handle, >> } >> >> if (insert_bh == parent_fe_bh) >> - status = ocfs2_journal_access_di(handle, >> + retval = ocfs2_journal_access_di(handle, >> INODE_CACHE(dir), >> insert_bh, >> OCFS2_JOURNAL_ACCESS_WRITE); >> else { >> - status = ocfs2_journal_access_db(handle, >> + retval = ocfs2_journal_access_db(handle, >> INODE_CACHE(dir), >> insert_bh, >> OCFS2_JOURNAL_ACCESS_WRITE); >> >> - if (ocfs2_dir_indexed(dir)) { >> - status = ocfs2_dx_dir_insert(dir, >> + if (!retval && ocfs2_dir_indexed(dir)) >> + retval = ocfs2_dx_dir_insert(dir, >> handle, >> lookup); >> - if (status) { >> - mlog_errno(status); >> - goto bail; >> - } >> - } >> + } >> + >> + if (retval) { >> + mlog_errno(retval); >> + goto bail; >> } >> >> /* By now the buffer is marked for journaling */ >> > >
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c index 990e8f7..a9513ff 100644 --- a/fs/ocfs2/dir.c +++ b/fs/ocfs2/dir.c @@ -1607,7 +1607,7 @@ int __ocfs2_add_entry(handle_t *handle, struct ocfs2_dir_entry *de, *de1; struct ocfs2_dinode *di = (struct ocfs2_dinode *)parent_fe_bh->b_data; struct super_block *sb = dir->i_sb; - int retval, status; + int retval; unsigned int size = sb->s_blocksize; struct buffer_head *insert_bh = lookup->dl_leaf_bh; char *data_start = insert_bh->b_data; @@ -1685,25 +1685,25 @@ int __ocfs2_add_entry(handle_t *handle, } if (insert_bh == parent_fe_bh) - status = ocfs2_journal_access_di(handle, + retval = ocfs2_journal_access_di(handle, INODE_CACHE(dir), insert_bh, OCFS2_JOURNAL_ACCESS_WRITE); else { - status = ocfs2_journal_access_db(handle, + retval = ocfs2_journal_access_db(handle, INODE_CACHE(dir), insert_bh, OCFS2_JOURNAL_ACCESS_WRITE); - if (ocfs2_dir_indexed(dir)) { - status = ocfs2_dx_dir_insert(dir, + if (!retval && ocfs2_dir_indexed(dir)) + retval = ocfs2_dx_dir_insert(dir, handle, lookup); - if (status) { - mlog_errno(status); - goto bail; - } - } + } + + if (retval) { + mlog_errno(retval); + goto bail; } /* By now the buffer is marked for journaling */
The use of 'status' in __ocfs2_add_entry() can return wrong value. Some functions' return value in __ocfs2_add_entry(), i.e ocfs2_journal_access_di() is saved to 'status'. But 'status' is not used in 'bail' label for returning result of __ocfs2_add_entry(). So use retval instead of status. Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com> --- This patch was came from 'https://lkml.org/lkml/2015/2/27/655' This patch was needed to test but I didn't have any environment for testing ocfs2 filesystem. But I have one, now. (I'm too busy to make this enviroment. And qemu for this fs is difficult for me. :-(, sorry for that) Briefly how to set my environment for testing this fs with qemu. 1. Getting and building linux kernel with linux-next branch for x86_64 qemu. And also options of ocfs2 related are enabled(built-in) 2. Makes own root file system with 'buildroot' project. 3. Getting and building ocfs2-tools. Then binaries after building this tool are moved my rootfs. 4. Makes dummy disk image(5G) which will be formatted in qemu. 5. Booting qemu with rootfs image and dummy disk image. 6. mkfs.ocfs2 --fs-feature=local <device> this maybe possilbe to mount standalone mode. 7. tunefs.ocfs2 --fs-features=indexed-dirs,noinline-data <device> 8. make a cluster and one node use o2cb_ctl tool. 9. o2cb service load and initialize # /etc/init.d/o2cb load && /etc/init.d/o2cb configure # /etc/init.d/o2cb online 10. mount ocfs2 # mount.ocfs2 <device> <some directory> And use GDB for debugging my patch path. Connect gdb with qemu and add breakpoint in __ocfs2_add_entry() of fs/ocfs2/dir.c And test my patch. # cd <some directory mounted with ocfs2> # mkdir <specific directory> This how-to is not written all my work, just briefly I said. fs/ocfs2/dir.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)