[PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata()

[PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata()

[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

Re: [PATCH] hfsplus: hfsplus_file_fsync() does not check for return value of sync_inode_metadata()

[Viacheslav Dubeyko]

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.