Btrfs devel - Dec 2013 - [PATCH] Btrfs: fix error check of btrfs_lookup_dentry()

Clean up btrfs_lookup_dentry() to never return NULL, but PTR_ERR(-ENOENT)
instead. This keeps the return value convention consistent.

Callers who use btrfs_lookup_dentry() require a trivial update.

create_snapshot() in particular looks like it can also lose a BUG_ON(!inode)
which is not really needed - there seems less harm in returning ENOENT to
userspace at that point in the stack than there is to crash the machine.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
 fs/btrfs/inode.c | 15 +++++++++++----
 fs/btrfs/ioctl.c | 13 ++++++++++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3b4ffaf..f9b45a7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4910,7 +4910,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir,
struct dentry *dentry)
 		return ERR_PTR(ret);
 
 	if (location.objectid == 0)
-		return NULL;
+		return ERR_PTR(-ENOENT);
 
 	if (location.type == BTRFS_INODE_ITEM_KEY) {
 		inode = btrfs_iget(dir->i_sb, &location, root, NULL);
@@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry *dentry)
 static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
 				   unsigned int flags)
 {
-	struct dentry *ret;
+	struct inode *inode;
 
-	ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
-	return ret;
+	inode = btrfs_lookup_dentry(dir, dentry);
+	if (IS_ERR(inode)) {
+		if (PTR_ERR(inode) == -ENOENT)
+			inode = NULL;
+		else
+			return ERR_CAST(inode);
+	}
+
+	return d_splice_alias(inode, dentry);
 }
 
 unsigned char btrfs_filetype_table[] = {
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9d46f60..716779c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -389,6 +389,7 @@ static noinline int create_subvol(struct inode *dir,
 	struct btrfs_root *new_root;
 	struct btrfs_block_rsv block_rsv;
 	struct timespec cur_time = CURRENT_TIME;
+	struct inode *inode;
 	int ret;
 	int err;
 	u64 objectid;
@@ -550,8 +551,14 @@ fail:
 	if (err && !ret)
 		ret = err;
 
-	if (!ret)
-		d_instantiate(dentry, btrfs_lookup_dentry(dir, dentry));
+	if (!ret) {
+		inode = btrfs_lookup_dentry(dir, dentry);
+		if (IS_ERR(inode)) {
+			ret = PTR_ERR(inode);
+			goto out;
+		}
+		d_instantiate(dentry, inode);
+	}
 out:
 	btrfs_subvolume_release_metadata(root, &block_rsv, qgroup_reserved);
 	return ret;
@@ -639,7 +646,7 @@ static int create_snapshot(struct btrfs_root *root, struct
inode *dir,
 		ret = PTR_ERR(inode);
 		goto fail;
 	}
-	BUG_ON(!inode);
+
 	d_instantiate(dentry, inode);
 	ret = 0;
 fail:

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh
wrote:
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry
*dentry)
>  static struct dentry *btrfs_lookup(struct inode *dir, struct dentry
*dentry,
>  				   unsigned int flags)
>  {
> -	struct dentry *ret;
> +	struct inode *inode;
>  
> -	ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
> -	return ret;
> +	inode = btrfs_lookup_dentry(dir, dentry);
> +	if (IS_ERR(inode)) {
> +		if (PTR_ERR(inode) == -ENOENT)
> +			inode = NULL;
> +		else
> +			return ERR_CAST(inode);
> +	}
> +
> +	return d_splice_alias(inode, dentry);
btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...)
and the expanded back and forth with the DCACHE_NEED_LOOKUP flag.

a66e7cc626f42de6c745963fe0d807518fa49d39 added
39e3c9553f34381a1b664c27b0c696a266a5735e removed

d_splice_alias has been made robust in
a9049376ee05bf966bfe2b081b5071326856890a
"make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)"

you can drop the error handling from btrfs_lookup completely.

The rest looks ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi, David,

On 2013/12/17 0:27, David Sterba wrote:
> On Fri, Dec 13, 2013 at 09:51:42AM +0900, Tsutomu Itoh wrote:
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4974,10 +4974,17 @@ static void btrfs_dentry_release(struct dentry
*dentry)
>>   static struct dentry *btrfs_lookup(struct inode *dir, struct dentry
*dentry,
>>   				   unsigned int flags)
>>   {
>> -	struct dentry *ret;
>> +	struct inode *inode;
>>
>> -	ret = d_splice_alias(btrfs_lookup_dentry(dir, dentry), dentry);
>> -	return ret;
>> +	inode = btrfs_lookup_dentry(dir, dentry);
>> +	if (IS_ERR(inode)) {
>> +		if (PTR_ERR(inode) == -ENOENT)
>> +			inode = NULL;
>> +		else
>> +			return ERR_CAST(inode);
>> +	}
>> +
>> +	return d_splice_alias(inode, dentry);
>
> btrfs_lookup used to be a simple d_splice_alias(btrfs_lookup_dentry ...)
> and the expanded back and forth with the DCACHE_NEED_LOOKUP flag.
>
> a66e7cc626f42de6c745963fe0d807518fa49d39 added
> 39e3c9553f34381a1b664c27b0c696a266a5735e removed
>
> d_splice_alias has been made robust in
> a9049376ee05bf966bfe2b081b5071326856890a
> "make d_splice_alias(ERR_PTR(err), dentry) = ERR_PTR(err)"
>
> you can drop the error handling from btrfs_lookup completely.
d_splice_alias() is called by the following formats if btrfs_lookup_dentry()
returns NULL before my patch is applied.
   d_splice_alias(NULL, dentry);
However, d_splice_alias() is called by the following formats when
becoming the same situation after my patch is applied.
   d_splice_alias(-ENOENT, dentry);

Therefore, I added the following error handling code.
+       if (IS_ERR(inode)) {
+               if (PTR_ERR(inode) == -ENOENT)
+                       inode = NULL;
+               else
+                       return ERR_CAST(inode);
+       }

Thanks,
Tsutomu
>
> The rest looks ok.
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html