diff mbox series

[RFC,v2,1/4] btrfs: factor out btrfs_link_subvol from create_subvol

Message ID 20180911112903.25985-2-lufq.fnst@cn.fujitsu.com (mailing list archive)
State New, archived
Headers show
Series undelete subvolume online version | expand

Commit Message

Lu Fengqi Sept. 11, 2018, 11:29 a.m. UTC 
The function btrfs_link_subvol is responsible to link the subvolume to
the specified directory, which is the opposite of what
btrfs_unlink_subvol does.

No functional change.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 23 deletions(-)

Comments

Qu Wenruo Sept. 11, 2018, 11:57 a.m. UTC | #1 
On 2018/9/11 下午7:29, Lu Fengqi wrote:
> The function btrfs_link_subvol is responsible to link the subvolume to
> the specified directory, which is the opposite of what
> btrfs_unlink_subvol does.
> 
> No functional change.
> 
> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>

The patch itself is OK.

Just small nitpicks inlined below.

> ---
>  fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 4905d13dee0a..1b03d07acde2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -542,6 +542,45 @@ int btrfs_is_empty_uuid(u8 *uuid)
>  	return 1;
>  }
>  
> +static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
> +			     struct inode *dir, u64 objectid, const char *name,
> +			     int namelen)
> +{
> +	struct btrfs_root *root = BTRFS_I(dir)->root;
> +	struct btrfs_key key;
> +	u64 index = 0;
> +	int ret;
> +
> +	/*
> +	 * insert the directory item
> +	 */
> +	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	key.objectid = objectid;
> +	key.type = BTRFS_ROOT_ITEM_KEY;
> +	key.offset = -1;
> +	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
> +				    BTRFS_FT_DIR, index);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
> +	ret = btrfs_update_inode(trans, root, dir);
> +	BUG_ON(ret);

What about clean up this BUG_ON()?

> +
> +	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
> +				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
> +	BUG_ON(ret);

And this one?

Thanks,
Qu

> +
> +	return ret;
> +}
> +
>  static noinline int create_subvol(struct inode *dir,
>  				  struct dentry *dentry,
>  				  const char *name, int namelen,
> @@ -563,7 +602,6 @@ static noinline int create_subvol(struct inode *dir,
>  	int err;
>  	u64 objectid;
>  	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
> -	u64 index = 0;
>  	uuid_le new_uuid;
>  
>  	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
> @@ -677,29 +715,9 @@ static noinline int create_subvol(struct inode *dir,
>  	new_root->highest_objectid = new_dirid;
>  	mutex_unlock(&new_root->objectid_mutex);
>  
> -	/*
> -	 * insert the directory item
> -	 */
> -	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto fail;
> -	}
> -
> -	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
> -				    BTRFS_FT_DIR, index);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> +	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
> +	if (ret)
>  		goto fail;
> -	}
> -
> -	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
> -	ret = btrfs_update_inode(trans, root, dir);
> -	BUG_ON(ret);
> -
> -	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
> -				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
> -	BUG_ON(ret);
>  
>  	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
>  				  BTRFS_UUID_KEY_SUBVOL, objectid);
>
Lu Fengqi Sept. 11, 2018, 12:10 p.m. UTC | #2 
On Tue, Sep 11, 2018 at 07:57:03PM +0800, Qu Wenruo wrote:
>
>
>On 2018/9/11 下午7:29, Lu Fengqi wrote:
>> The function btrfs_link_subvol is responsible to link the subvolume to
>> the specified directory, which is the opposite of what
>> btrfs_unlink_subvol does.
>> 
>> No functional change.
>> 
>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>
>The patch itself is OK.
>
>Just small nitpicks inlined below.
>
>> ---
>>  fs/btrfs/ioctl.c | 64 +++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 41 insertions(+), 23 deletions(-)
>> 
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 4905d13dee0a..1b03d07acde2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -542,6 +542,45 @@ int btrfs_is_empty_uuid(u8 *uuid)
>>  	return 1;
>>  }
>>  
>> +static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
>> +			     struct inode *dir, u64 objectid, const char *name,
>> +			     int namelen)
>> +{
>> +	struct btrfs_root *root = BTRFS_I(dir)->root;
>> +	struct btrfs_key key;
>> +	u64 index = 0;
>> +	int ret;
>> +
>> +	/*
>> +	 * insert the directory item
>> +	 */
>> +	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		return ret;
>> +	}
>> +
>> +	key.objectid = objectid;
>> +	key.type = BTRFS_ROOT_ITEM_KEY;
>> +	key.offset = -1;
>> +	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
>> +				    BTRFS_FT_DIR, index);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		return ret;
>> +	}
>> +
>> +	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
>> +	ret = btrfs_update_inode(trans, root, dir);
>> +	BUG_ON(ret);
>
>What about clean up this BUG_ON()?
>
>> +
>> +	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
>> +				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
>> +	BUG_ON(ret);
>
>And this one?

Sorry, this makes you confused. This is exactly the cleanup done by Patch 2, because
I want to just move the code in Patch 1.

Thanks,
Lu

>
>Thanks,
>Qu
>
>> +
>> +	return ret;
>> +}
>> +
>>  static noinline int create_subvol(struct inode *dir,
>>  				  struct dentry *dentry,
>>  				  const char *name, int namelen,
>> @@ -563,7 +602,6 @@ static noinline int create_subvol(struct inode *dir,
>>  	int err;
>>  	u64 objectid;
>>  	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
>> -	u64 index = 0;
>>  	uuid_le new_uuid;
>>  
>>  	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
>> @@ -677,29 +715,9 @@ static noinline int create_subvol(struct inode *dir,
>>  	new_root->highest_objectid = new_dirid;
>>  	mutex_unlock(&new_root->objectid_mutex);
>>  
>> -	/*
>> -	 * insert the directory item
>> -	 */
>> -	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
>> -	if (ret) {
>> -		btrfs_abort_transaction(trans, ret);
>> -		goto fail;
>> -	}
>> -
>> -	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
>> -				    BTRFS_FT_DIR, index);
>> -	if (ret) {
>> -		btrfs_abort_transaction(trans, ret);
>> +	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
>> +	if (ret)
>>  		goto fail;
>> -	}
>> -
>> -	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
>> -	ret = btrfs_update_inode(trans, root, dir);
>> -	BUG_ON(ret);
>> -
>> -	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
>> -				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
>> -	BUG_ON(ret);
>>  
>>  	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
>>  				  BTRFS_UUID_KEY_SUBVOL, objectid);
>> 
>
diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4905d13dee0a..1b03d07acde2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -542,6 +542,45 @@  int btrfs_is_empty_uuid(u8 *uuid)
 	return 1;
 }
 
+static int btrfs_link_subvol(struct btrfs_trans_handle *trans,
+			     struct inode *dir, u64 objectid, const char *name,
+			     int namelen)
+{
+	struct btrfs_root *root = BTRFS_I(dir)->root;
+	struct btrfs_key key;
+	u64 index = 0;
+	int ret;
+
+	/*
+	 * insert the directory item
+	 */
+	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
+
+	key.objectid = objectid;
+	key.type = BTRFS_ROOT_ITEM_KEY;
+	key.offset = -1;
+	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
+				    BTRFS_FT_DIR, index);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		return ret;
+	}
+
+	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
+	ret = btrfs_update_inode(trans, root, dir);
+	BUG_ON(ret);
+
+	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
+				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
+	BUG_ON(ret);
+
+	return ret;
+}
+
 static noinline int create_subvol(struct inode *dir,
 				  struct dentry *dentry,
 				  const char *name, int namelen,
@@ -563,7 +602,6 @@  static noinline int create_subvol(struct inode *dir,
 	int err;
 	u64 objectid;
 	u64 new_dirid = BTRFS_FIRST_FREE_OBJECTID;
-	u64 index = 0;
 	uuid_le new_uuid;
 
 	root_item = kzalloc(sizeof(*root_item), GFP_KERNEL);
@@ -677,29 +715,9 @@  static noinline int create_subvol(struct inode *dir,
 	new_root->highest_objectid = new_dirid;
 	mutex_unlock(&new_root->objectid_mutex);
 
-	/*
-	 * insert the directory item
-	 */
-	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
-		goto fail;
-	}
-
-	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
-				    BTRFS_FT_DIR, index);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
+	ret = btrfs_link_subvol(trans, dir, objectid, name, namelen);
+	if (ret)
 		goto fail;
-	}
-
-	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
-	ret = btrfs_update_inode(trans, root, dir);
-	BUG_ON(ret);
-
-	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
-				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
-	BUG_ON(ret);
 
 	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
 				  BTRFS_UUID_KEY_SUBVOL, objectid);