btrfs: qgroup: Remove qgroup item along with subvolume deletion
diff mbox series

Message ID eeaa9ab1-f6a5-9043-9b5e-cc9acf82ca47@jp.fujitsu.com
State New
Headers show
Series
  • btrfs: qgroup: Remove qgroup item along with subvolume deletion
Related show

Commit Message

Misono Tomohiro Aug. 3, 2018, 4:08 a.m. UTC
When qgroup is on, subvolume deletion does not remove qgroup items
of the subvolume (qgroup info, limits, relation) from quota tree and
they need to get removed manually by "btrfs qgroup destroy".

Since level 0 qgroup cannot be used/inherited by any other subvolume,
let's remove them automatically when subvolume is deleted.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---

I don't see any reason to keep these items after subvolume deletion,
but is there something I'm missing?

 fs/btrfs/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Qu Wenruo Aug. 3, 2018, 4:17 a.m. UTC | #1
On 2018年08月03日 12:08, Misono Tomohiro wrote:
> When qgroup is on, subvolume deletion does not remove qgroup items
> of the subvolume (qgroup info, limits, relation) from quota tree and
> they need to get removed manually by "btrfs qgroup destroy".
> 
> Since level 0 qgroup cannot be used/inherited by any other subvolume,
> let's remove them automatically when subvolume is deleted.
> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> ---
> 
> I don't see any reason to keep these items after subvolume deletion,
> but is there something I'm missing?
> 
>  fs/btrfs/inode.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3f51ddc18f98..4ec60a1b53a3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>  		}
>  	}
>  
> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);

According to the caller, it only unlinks the subvolume without really
delete the whole subvolume.

I'm wondering if we should call btrfs_remove_qgroup() only after we have
deleted the whole subvolume, e.g inside btrfs_drop_snapshot().

Thanks,
Qu

> +	if (ret == -EINVAL || ret == -ENOENT)
> +		ret = 0;
> +
>  out_end_trans:
>  	trans->block_rsv = NULL;
>  	trans->bytes_reserved = 0;
>
Lu Fengqi Aug. 3, 2018, 4:23 a.m. UTC | #2
On Fri, Aug 03, 2018 at 12:17:26PM +0800, Qu Wenruo wrote:
>
>
>On 2018年08月03日 12:08, Misono Tomohiro wrote:
>> When qgroup is on, subvolume deletion does not remove qgroup items
>> of the subvolume (qgroup info, limits, relation) from quota tree and
>> they need to get removed manually by "btrfs qgroup destroy".
>> 
>> Since level 0 qgroup cannot be used/inherited by any other subvolume,
>> let's remove them automatically when subvolume is deleted.
>> 
>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>> ---
>> 
>> I don't see any reason to keep these items after subvolume deletion,
>> but is there something I'm missing?
>> 
>>  fs/btrfs/inode.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 3f51ddc18f98..4ec60a1b53a3 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>>  		}
>>  	}
>>  
>> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
>
>According to the caller, it only unlinks the subvolume without really
>delete the whole subvolume.
>
>I'm wondering if we should call btrfs_remove_qgroup() only after we have
>deleted the whole subvolume, e.g inside btrfs_drop_snapshot().

I agree with Qu's point, because the ongoing online undelete subvolume will
be able to recover the subvolume which is intact ondisk, including the qgroup.
Misono Tomohiro Aug. 3, 2018, 4:27 a.m. UTC | #3
On 2018/08/03 13:23, Lu Fengqi wrote:
> On Fri, Aug 03, 2018 at 12:17:26PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年08月03日 12:08, Misono Tomohiro wrote:
>>> When qgroup is on, subvolume deletion does not remove qgroup items
>>> of the subvolume (qgroup info, limits, relation) from quota tree and
>>> they need to get removed manually by "btrfs qgroup destroy".
>>>
>>> Since level 0 qgroup cannot be used/inherited by any other subvolume,
>>> let's remove them automatically when subvolume is deleted.
>>>
>>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
>>> ---
>>>
>>> I don't see any reason to keep these items after subvolume deletion,
>>> but is there something I'm missing?
>>>
>>>  fs/btrfs/inode.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index 3f51ddc18f98..4ec60a1b53a3 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -4372,6 +4372,10 @@ int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>>>  		}
>>>  	}
>>>  
>>> +	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
>>
>> According to the caller, it only unlinks the subvolume without really
>> delete the whole subvolume.
>>
>> I'm wondering if we should call btrfs_remove_qgroup() only after we have
>> deleted the whole subvolume, e.g inside btrfs_drop_snapshot().
> 
> I agree with Qu's point, because the ongoing online undelete subvolume will
> be able to recover the subvolume which is intact ondisk, including the qgroup.
> 

Thanks to both of you, I'll update the patch.
Misono

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

Patch
diff mbox series

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3f51ddc18f98..4ec60a1b53a3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4372,6 +4372,10 @@  int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
 		}
 	}
 
+	ret = btrfs_remove_qgroup(trans, dest->root_key.objectid);
+	if (ret == -EINVAL || ret == -ENOENT)
+		ret = 0;
+
 out_end_trans:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;