| Message ID | 53312AD6.4080607@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
These changes looks good to me. However ocfs2_read_blocks and ocfs2_read_blocks_sync needs the same fix ? :) On 03/25/2014 12:05 AM, alex chen wrote: > Do not put bh when buffer_uptodate failed in ocfs2_write_block and > ocfs2_write_super_or_backup, because it will put bh in b_end_io. > Otherwise it will hit a warning "VFS: brelse: Trying to free free > buffer". > > Signed-off-by: Alex Chen <alex.chen@huawei.com> > Reviewed-by: Joseph Qi <joseph.qi@huawei.com> > --- > fs/ocfs2/buffer_head_io.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c > index 5b704c6..1edcb14 100644 > --- a/fs/ocfs2/buffer_head_io.c > +++ b/fs/ocfs2/buffer_head_io.c > @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, > * information for this bh as it's not marked locally > * uptodate. */ > ret = -EIO; > - put_bh(bh); > mlog_errno(ret); > } > > @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, > > if (!buffer_uptodate(bh)) { > ret = -EIO; > - put_bh(bh); > mlog_errno(ret); > } >
On 2014/3/26 2:45, Srinivas Eeda wrote: > These changes looks good to me. However ocfs2_read_blocks and > ocfs2_read_blocks_sync needs the same fix ? :) There is no need to do this in ocfs2_read_blocks and ocfs2_read_blocks_sync, because bh will be set to NULL after put_bh, and brelse will handle it. > > On 03/25/2014 12:05 AM, alex chen wrote: >> Do not put bh when buffer_uptodate failed in ocfs2_write_block and >> ocfs2_write_super_or_backup, because it will put bh in b_end_io. >> Otherwise it will hit a warning "VFS: brelse: Trying to free free >> buffer". >> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> Reviewed-by: Joseph Qi <joseph.qi@huawei.com> >> --- >> fs/ocfs2/buffer_head_io.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c >> index 5b704c6..1edcb14 100644 >> --- a/fs/ocfs2/buffer_head_io.c >> +++ b/fs/ocfs2/buffer_head_io.c >> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, >> * information for this bh as it's not marked locally >> * uptodate. */ >> ret = -EIO; >> - put_bh(bh); >> mlog_errno(ret); >> } >> >> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, >> >> if (!buffer_uptodate(bh)) { >> ret = -EIO; >> - put_bh(bh); >> mlog_errno(ret); >> } >> > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel > >
Thanks for explaining Reviewed-by: Srinivas Eeda <srinivas.eeda@oracle.com> On 03/25/2014 06:25 PM, alex chen wrote: > On 2014/3/26 2:45, Srinivas Eeda wrote: >> These changes looks good to me. However ocfs2_read_blocks and >> ocfs2_read_blocks_sync needs the same fix ? :) > There is no need to do this in ocfs2_read_blocks and > ocfs2_read_blocks_sync, because bh will be set to NULL after put_bh, > and brelse will handle it. > >> On 03/25/2014 12:05 AM, alex chen wrote: >>> Do not put bh when buffer_uptodate failed in ocfs2_write_block and >>> ocfs2_write_super_or_backup, because it will put bh in b_end_io. >>> Otherwise it will hit a warning "VFS: brelse: Trying to free free >>> buffer". >>> >>> Signed-off-by: Alex Chen <alex.chen@huawei.com> >>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com> >>> --- >>> fs/ocfs2/buffer_head_io.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c >>> index 5b704c6..1edcb14 100644 >>> --- a/fs/ocfs2/buffer_head_io.c >>> +++ b/fs/ocfs2/buffer_head_io.c >>> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, >>> * information for this bh as it's not marked locally >>> * uptodate. */ >>> ret = -EIO; >>> - put_bh(bh); >>> mlog_errno(ret); >>> } >>> >>> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, >>> >>> if (!buffer_uptodate(bh)) { >>> ret = -EIO; >>> - put_bh(bh); >>> mlog_errno(ret); >>> } >>> >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> >>
On Tue, Mar 25, 2014 at 03:05:58PM +0800, alex chen wrote: > Do not put bh when buffer_uptodate failed in ocfs2_write_block and > ocfs2_write_super_or_backup, because it will put bh in b_end_io. > Otherwise it will hit a warning "VFS: brelse: Trying to free free > buffer". > > Signed-off-by: Alex Chen <alex.chen@huawei.com> > Reviewed-by: Joseph Qi <joseph.qi@huawei.com> Good catch. Can you tell me what testing or workload found this issue? Just for future reference. Acked-by: Joel Becker <jlbec@evilplan.org> > --- > fs/ocfs2/buffer_head_io.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c > index 5b704c6..1edcb14 100644 > --- a/fs/ocfs2/buffer_head_io.c > +++ b/fs/ocfs2/buffer_head_io.c > @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, > * information for this bh as it's not marked locally > * uptodate. */ > ret = -EIO; > - put_bh(bh); > mlog_errno(ret); > } > > @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, > > if (!buffer_uptodate(bh)) { > ret = -EIO; > - put_bh(bh); > mlog_errno(ret); > } > > -- > 1.8.4.3 > > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel
On 2014/3/27 10:22, Joel Becker wrote: > On Tue, Mar 25, 2014 at 03:05:58PM +0800, alex chen wrote: >> Do not put bh when buffer_uptodate failed in ocfs2_write_block and >> ocfs2_write_super_or_backup, because it will put bh in b_end_io. >> Otherwise it will hit a warning "VFS: brelse: Trying to free free >> buffer". >> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> Reviewed-by: Joseph Qi <joseph.qi@huawei.com> > > Good catch. Can you tell me what testing or workload found this issue? > Just for future reference. > > Acked-by: Joel Becker <jlbec@evilplan.org> > This issue was found when resizing volume. During the resize, storage link was not steady and continuing up and down. Uptodate buffer failed because of EIO in ocfs2_write_super_or_backup and then this warning occurs. >> --- >> fs/ocfs2/buffer_head_io.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c >> index 5b704c6..1edcb14 100644 >> --- a/fs/ocfs2/buffer_head_io.c >> +++ b/fs/ocfs2/buffer_head_io.c >> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, >> * information for this bh as it's not marked locally >> * uptodate. */ >> ret = -EIO; >> - put_bh(bh); >> mlog_errno(ret); >> } >> >> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, >> >> if (!buffer_uptodate(bh)) { >> ret = -EIO; >> - put_bh(bh); >> mlog_errno(ret); >> } >> >> -- >> 1.8.4.3 >> >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >
diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c index 5b704c6..1edcb14 100644 --- a/fs/ocfs2/buffer_head_io.c +++ b/fs/ocfs2/buffer_head_io.c @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh, * information for this bh as it's not marked locally * uptodate. */ ret = -EIO; - put_bh(bh); mlog_errno(ret); } @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb, if (!buffer_uptodate(bh)) { ret = -EIO; - put_bh(bh); mlog_errno(ret); }