[PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[Vyacheslav Dubeyko]

Hi,

The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching.

With the best regards,
Vyacheslav Dubeyko.
--
From: Vyacheslav Dubeyko <slava@dubeyko.com>
Subject: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error

The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching. It is added also logic of processing errors of hfs_btree_write() in hfsplus_system_write_inode() with message about b-tree writing failure.

Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfsplus/btree.c      |    5 +++--
 fs/hfsplus/hfsplus_fs.h |    2 +-
 fs/hfsplus/super.c      |   12 ++++++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 21023d9..685d07d 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -159,7 +159,7 @@ void hfs_btree_close(struct hfs_btree *tree)
 	kfree(tree);
 }
 
-void hfs_btree_write(struct hfs_btree *tree)
+int hfs_btree_write(struct hfs_btree *tree)
 {
 	struct hfs_btree_header_rec *head;
 	struct hfs_bnode *node;
@@ -168,7 +168,7 @@ void hfs_btree_write(struct hfs_btree *tree)
 	node = hfs_bnode_find(tree, 0);
 	if (IS_ERR(node))
 		/* panic? */
-		return;
+		return -EIO;
 	/* Load the header */
 	page = node->page[0];
 	head = (struct hfs_btree_header_rec *)(kmap(page) +
@@ -186,6 +186,7 @@ void hfs_btree_write(struct hfs_btree *tree)
 	kunmap(page);
 	set_page_dirty(page);
 	hfs_bnode_put(node);
+	return 0;
 }
 
 static struct hfs_bnode *hfs_bmap_new_bmap(struct hfs_bnode *prev, u32 idx)
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index c571de2..a6da86b 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -335,7 +335,7 @@ int hfsplus_block_free(struct super_block *, u32, u32);
 /* btree.c */
 struct hfs_btree *hfs_btree_open(struct super_block *, u32);
 void hfs_btree_close(struct hfs_btree *);
-void hfs_btree_write(struct hfs_btree *);
+int hfs_btree_write(struct hfs_btree *);
 struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *);
 void hfs_bmap_free(struct hfs_bnode *);
 
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 811a84d..3033b73 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -99,6 +99,7 @@ static int hfsplus_system_write_inode(struct inode *inode)
 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
 	struct hfsplus_fork_raw *fork;
 	struct hfs_btree *tree = NULL;
+	int err;
 
 	switch (inode->i_ino) {
 	case HFSPLUS_EXT_CNID:
@@ -127,8 +128,15 @@ static int hfsplus_system_write_inode(struct inode *inode)
 		hfsplus_mark_mdb_dirty(inode->i_sb);
 	}
 	hfsplus_inode_write_fork(inode, fork);
-	if (tree)
-		hfs_btree_write(tree);
+	if (tree) {
+		err = hfs_btree_write(tree);
+		if (err) {
+			printk(KERN_ERR "hfs: unable to write b-tree\n");
+			dprint(DBG_INODE, "hfsplus_system_write_inode: %lu\n",
+				inode->i_ino);
+			return err;
+		}
+	}
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[Andrew Morton]

On Fri, 30 Nov 2012 14:49:26 +0400
Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

> Hi,
> 
> The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching.
> 
> With the best regards,
> Vyacheslav Dubeyko.
> --
> From: Vyacheslav Dubeyko <slava@dubeyko.com>
> Subject: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error
> 
> The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching. It is added also logic of processing errors of hfs_btree_write() in hfsplus_system_write_inode() with message about b-tree writing failure.
> 
> Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>

We don't need two changelogs for each patch ;) The patch's formal
changelog text should be sufficient.

> fs/hfsplus/btree.c      |    5 +++--
> fs/hfsplus/hfsplus_fs.h |    2 +-
> fs/hfsplus/super.c      |   12 ++++++++++--
> 3 files changed, 14 insertions(+), 5 deletions(-)

I modified a couple of things, please review:

From: Andrew Morton <akpm@linux-foundation.org>
Subject: hfsplus-rework-processing-of-hfs_btree_write-returned-error-fix

reduce scope of `err', print errno on error

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/hfsplus/super.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/fs/hfsplus/super.c~hfsplus-rework-processing-of-hfs_btree_write-returned-error-fix
+++ a/fs/hfsplus/super.c
@@ -100,7 +100,6 @@ static int hfsplus_system_write_inode(st
 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
 	struct hfsplus_fork_raw *fork;
 	struct hfs_btree *tree = NULL;
-	int err;
 
 	switch (inode->i_ino) {
 	case HFSPLUS_EXT_CNID:
@@ -131,9 +130,10 @@ static int hfsplus_system_write_inode(st
 	}
 	hfsplus_inode_write_fork(inode, fork);
 	if (tree) {
-		err = hfs_btree_write(tree);
+		int err = hfs_btree_write(tree);
 		if (err) {
-			printk(KERN_ERR "hfs: unable to write b-tree\n");
+			printk(KERN_ERR "hfs: unable to write b-tree: %d\n",
+				err);
 			dprint(DBG_INODE, "hfsplus_system_write_inode: %lu\n",
 				inode->i_ino);
 			return err;
_

Re: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[Hin-Tak Leung]

Acked, and also a comment towards the end (so please keep reading beyond the Ack!).

--- On Fri, 30/11/12, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

> Hi,
> 
> The patch adds in hfs_btree_write() returning of -EIO in the
> case of failing of b-tree's node searching.
> 
> With the best regards,
> Vyacheslav Dubeyko.
> --
> From: Vyacheslav Dubeyko <slava@dubeyko.com>
> Subject: [PATCH] hfsplus: rework processing of
> hfs_btree_write() returned error
> 
> The patch adds in hfs_btree_write() returning of -EIO in the
> case of failing of b-tree's node searching. It is added also
> logic of processing errors of hfs_btree_write() in
> hfsplus_system_write_inode() with message about b-tree
> writing failure.
> 
> Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Acked-by: Hin-Tak Leung <htl10@users.sourceforge.net>

> ---
>  fs/hfsplus/btree.c      |    5
> +++--
>  fs/hfsplus/hfsplus_fs.h |    2 +-
>  fs/hfsplus/super.c     
> |   12 ++++++++++--
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 21023d9..685d07d 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -159,7 +159,7 @@ void hfs_btree_close(struct hfs_btree
> *tree)
>      kfree(tree);
>  }
>  
> -void hfs_btree_write(struct hfs_btree *tree)
> +int hfs_btree_write(struct hfs_btree *tree)
>  {
>      struct hfs_btree_header_rec *head;
>      struct hfs_bnode *node;
> @@ -168,7 +168,7 @@ void hfs_btree_write(struct hfs_btree
> *tree)
>      node = hfs_bnode_find(tree, 0);
>      if (IS_ERR(node))
>          /* panic? */
> -        return;
> +        return -EIO;
>      /* Load the header */
>      page = node->page[0];
>      head = (struct hfs_btree_header_rec
> *)(kmap(page) +
> @@ -186,6 +186,7 @@ void hfs_btree_write(struct hfs_btree
> *tree)
>      kunmap(page);
>      set_page_dirty(page);
>      hfs_bnode_put(node);
> +    return 0;
>  }
>  
>  static struct hfs_bnode *hfs_bmap_new_bmap(struct hfs_bnode
> *prev, u32 idx)
> diff --git a/fs/hfsplus/hfsplus_fs.h
> b/fs/hfsplus/hfsplus_fs.h
> index c571de2..a6da86b 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -335,7 +335,7 @@ int hfsplus_block_free(struct
> super_block *, u32, u32);
>  /* btree.c */
>  struct hfs_btree *hfs_btree_open(struct super_block *,
> u32);
>  void hfs_btree_close(struct hfs_btree *);
> -void hfs_btree_write(struct hfs_btree *);
> +int hfs_btree_write(struct hfs_btree *);
>  struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *);
>  void hfs_bmap_free(struct hfs_bnode *);
>  
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 811a84d..3033b73 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -99,6 +99,7 @@ static int
> hfsplus_system_write_inode(struct inode *inode)
>      struct hfsplus_vh *vhdr =
> sbi->s_vhdr;
>      struct hfsplus_fork_raw *fork;
>      struct hfs_btree *tree = NULL;
> +    int err;
>  
>      switch (inode->i_ino) {
>      case HFSPLUS_EXT_CNID:
> @@ -127,8 +128,15 @@ static int
> hfsplus_system_write_inode(struct inode *inode)
>         
> hfsplus_mark_mdb_dirty(inode->i_sb);
>      }
>      hfsplus_inode_write_fork(inode, fork);
> -    if (tree)
> -       
> hfs_btree_write(tree);
> +    if (tree) {
> +        err =
> hfs_btree_write(tree);
> +        if (err) {
> +           
> printk(KERN_ERR "hfs: unable to write b-tree\n");
> +           
> dprint(DBG_INODE, "hfsplus_system_write_inode: %lu\n",
> +           
>     inode->i_ino);
> +           
> return err;
> +        }
> +    }
>      return 0;
>  }
>  

There is a small issue which crossed my mind when I first noticed with:
"hfsplus: rework processing errors in hfsplus_free_extents()"

It is a printk() followed by dprint() with additional information, on error. Because dprint() needs a compile-time change (or at least, echo'ing into debugfs if switched to dynamic-debug). This means the additional information would not be seen the first time the error occurred. I am wondering whether the additional info should be included in the printk - in a short-hand form, maybe?

Hin-Tak

--
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

Re: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[Vyacheslav Dubeyko]

On Dec 1, 2012, at 12:56 AM, Andrew Morton wrote:

> On Fri, 30 Nov 2012 14:49:26 +0400
> Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> 
>> Hi,
>> 
>> The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching.
>> 
>> With the best regards,
>> Vyacheslav Dubeyko.
>> --
>> From: Vyacheslav Dubeyko <slava@dubeyko.com>
>> Subject: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error
>> 
>> The patch adds in hfs_btree_write() returning of -EIO in the case of failing of b-tree's node searching. It is added also logic of processing errors of hfs_btree_write() in hfsplus_system_write_inode() with message about b-tree writing failure.
>> 
>> Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
> 
> We don't need two changelogs for each patch ;) The patch's formal
> changelog text should be sufficient.
> 

Ok. :-)

>> fs/hfsplus/btree.c      |    5 +++--
>> fs/hfsplus/hfsplus_fs.h |    2 +-
>> fs/hfsplus/super.c      |   12 ++++++++++--
>> 3 files changed, 14 insertions(+), 5 deletions(-)
> 
> I modified a couple of things, please review:
> 

I think that it is a good correction. I agree.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

With the best regards,
Vyacheslav Dubeyko.

> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: hfsplus-rework-processing-of-hfs_btree_write-returned-error-fix
> 
> reduce scope of `err', print errno on error
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Hin-Tak Leung <htl10@users.sourceforge.net>
> Cc: Vyacheslav Dubeyko <slava@dubeyko.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
> fs/hfsplus/super.c |    6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> --- a/fs/hfsplus/super.c~hfsplus-rework-processing-of-hfs_btree_write-returned-error-fix
> +++ a/fs/hfsplus/super.c
> @@ -100,7 +100,6 @@ static int hfsplus_system_write_inode(st
> 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
> 	struct hfsplus_fork_raw *fork;
> 	struct hfs_btree *tree = NULL;
> -	int err;
> 
> 	switch (inode->i_ino) {
> 	case HFSPLUS_EXT_CNID:
> @@ -131,9 +130,10 @@ static int hfsplus_system_write_inode(st
> 	}
> 	hfsplus_inode_write_fork(inode, fork);
> 	if (tree) {
> -		err = hfs_btree_write(tree);
> +		int err = hfs_btree_write(tree);
> 		if (err) {
> -			printk(KERN_ERR "hfs: unable to write b-tree\n");
> +			printk(KERN_ERR "hfs: unable to write b-tree: %d\n",
> +				err);
> 			dprint(DBG_INODE, "hfsplus_system_write_inode: %lu\n",
> 				inode->i_ino);
> 			return err;
> _
> 
> --
> 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

Re: [PATCH] hfsplus: rework processing of hfs_btree_write() returned error

[Vyacheslav Dubeyko]

On Sat, 2012-12-01 at 12:28 +0000, Hin-Tak Leung wrote:

[snip]
>         if (err) {
> > +           
> > printk(KERN_ERR "hfs: unable to write b-tree\n");
> > +           
> > dprint(DBG_INODE, "hfsplus_system_write_inode: %lu\n",
> > +           
> >     inode->i_ino);
> > +           
> > return err;
> > +        }
> > +    }
> >      return 0;
> >  }
> >  
> 
> There is a small issue which crossed my mind when I first noticed with:
> "hfsplus: rework processing errors in hfsplus_free_extents()"
> 
> It is a printk() followed by dprint() with additional information, on error. Because dprint() needs a compile-time change (or at least, echo'ing into debugfs if switched to dynamic-debug). This means the additional information would not be seen the first time the error occurred. I am wondering whether the additional info should be included in the printk - in a short-hand form, maybe?
> 

I think that your remark is reasonable. I'll prepare the patch.

Thanks,
Vyacheslav Dubeyko.

> Hin-Tak
> 
> --
> 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