* [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata()
@ 2015-03-23 15:33 Changwoo Min
2015-03-23 17:16 ` Viacheslav Dubeyko
0 siblings, 1 reply; 2+ messages in thread
From: Changwoo Min @ 2015-03-23 15:33 UTC (permalink / raw
To: viro, linux-fsdevel, linux-kernel
Cc: taesoo, changwoo, sanidhya, blee, Changwoo Min
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;
+ }
/*
* And explicitly write out the btrees.
--
1.9.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata()
2015-03-23 15:33 [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata() Changwoo Min
@ 2015-03-23 17:16 ` Viacheslav Dubeyko
0 siblings, 0 replies; 2+ messages in thread
From: Viacheslav Dubeyko @ 2015-03-23 17:16 UTC (permalink / raw
To: Changwoo Min
Cc: viro, linux-fsdevel, linux-kernel, taesoo, changwoo, sanidhya,
blee
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-03-23 17:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-23 15:33 [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata() Changwoo Min
2015-03-23 17:16 ` Viacheslav Dubeyko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.