| Message ID | 1427124798-17530-1-git-send-email-changwoo.m@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-03-23 at 11:33 -0400, Changwoo Min wrote: > hfsplus_file_fsync() siliently ignores the return value of sync_inode_metadata(). > If an error occurs at sync_inode_metadata() and subsequent updates of other file > system metadata (b-trees) succeed, file system metadata will be inconsistent. > > Signed-off-by: Changwoo Min <changwoo.m@gmail.com> > --- > fs/hfsplus/inode.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c > index 0cf786f..9444719 100644 > --- a/fs/hfsplus/inode.c > +++ b/fs/hfsplus/inode.c > @@ -286,7 +286,11 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end, > /* > * Sync inode metadata into the catalog and extent trees. > */ > - sync_inode_metadata(inode, 1); > + error = sync_inode_metadata(inode, 1); > + if (error) { > + mutex_unlock(&inode->i_mutex); > + return error; > + } Thank you for suggestion. But I think that it's not proper fix. Trying to break the flushing is not good idea. Even if we have some troubles with sync_inode_metadata() call then it makes sense to flush all other dirty pages. Of course, we will have corrupted file system but to fix the corrupted state is responsibility of fsck tool. From my point of view, to try to flush as many as possible dirty pages is good strategy in the case of troubles during flushing. Because anyway we will have corrupted file system. Moreover, leaving memory pages in dirty state can hang writeback thread in the case of improper processing of dirty pages on file system driver in the case of likewise issue. I am not ready to say right now how proper hfsplus driver will process "ignored" dirty memory pages in writepage() call by writeback thread in the environment of such solution. Could you prove that your solution is right way? How it can be right solution? Could you describe some issue and how this change fixes the issue? Thanks, Vyacheslav Dubeyko. > > /* > * And explicitly write out the btrees. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index 0cf786f..9444719 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -286,7 +286,11 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end, /* * Sync inode metadata into the catalog and extent trees. */ - sync_inode_metadata(inode, 1); + error = sync_inode_metadata(inode, 1); + if (error) { + mutex_unlock(&inode->i_mutex); + return error; + } /* * And explicitly write out the btrees.
hfsplus_file_fsync() siliently ignores the return value of sync_inode_metadata(). If an error occurs at sync_inode_metadata() and subsequent updates of other file system metadata (b-trees) succeed, file system metadata will be inconsistent. Signed-off-by: Changwoo Min <changwoo.m@gmail.com> --- fs/hfsplus/inode.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)