On 2018/03/23 18:09, Nikolay Borisov wrote:
>
>
> On 23.03.2018 10:28, Misono Tomohiro wrote:
>> This patch changes the behavior of rmdir(2) to allow it to delete
>> an empty subvolume.
>>
>> In order to do that the core logic of subvolume deletion is moved from
>> ioctl.c to inode.c and named as btrfs_delete_subvolume(), which is
>> also called in the btrfs_rmdir() if a directory is an empty subvolume.
>>
>> Note that call of d_delete() is not included in the
>> btrfs_delete_subvolume(); they are not needed for rmdir(2) because
>> d_delete() will be called in vfs layer later.
>>
>> btrfs_delete_subvolume() requires inode_lock for both @dir and @dentry.
>> For rmdir(2) it is already acquired in vfs layer before calling
>> btrfs_rmdir().
>>
>> Also, btrfs_unlink_subvol() is no longer called outside of inode.c
>> and is changed to a static function.
>>
>> Signed-off-by: Tomohiro Misono <misono.tomohiro@xxxxxxxxxxxxxx>
>
> Do you intend to submit some xfstests to ensure the behavior is correct,
> ie we can't delete any populated subvolumes etc?
Not yet, but I will make fstests later.
>
> Also in the future when you do bulk copy/paste or introducing new
> function do it in the following way:
>
> 1. Introduce new functions
>
> 2. Replace current usage with new functions
>
> 3. Delete existing functions.
>
> That makes review a lot easier. Now I have to either trust you didn't
> make some subtle changes in the newly introduced function or start
> comparing them side-by-side with the old ones and this is cumbersome.
Understood. I will try to split the patch and send again.
Thanks,
Tomohiro Misono
>
>> ---
>> fs/btrfs/ctree.h | 5 +-
>> fs/btrfs/inode.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> fs/btrfs/ioctl.c | 185 +--------------------------------------------------
>> 3 files changed, 200 insertions(+), 189 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index da308774b8a4..6f23f0694ac3 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -3162,10 +3162,7 @@ int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
>> int btrfs_add_link(struct btrfs_trans_handle *trans,
>> struct btrfs_inode *parent_inode, struct btrfs_inode *inode,
>> const char *name, int name_len, int add_backref, u64 index);
>> -int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
>> - struct btrfs_root *root,
>> - struct inode *dir, u64 objectid,
>> - const char *name, int name_len);
>> +int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry);
>> int btrfs_truncate_block(struct inode *inode, loff_t from, loff_t len,
>> int front);
>> int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index f53470112670..c23e41e6cdfe 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4252,7 +4252,7 @@ static int btrfs_unlink(struct inode *dir, struct dentry *dentry)
>> return ret;
>> }
>>
>> -int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
>> +static int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
>> struct btrfs_root *root,
>> struct inode *dir, u64 objectid,
>> const char *name, int name_len)
>> @@ -4333,6 +4333,201 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
>> return ret;
>> }
>>
>> +/*
>> + * helper to check if the subvolume references other subvolumes
>> + */
>> +static noinline int may_destroy_subvol(struct btrfs_root *root)
>> +{
>> + struct btrfs_fs_info *fs_info = root->fs_info;
>> + struct btrfs_path *path;
>> + struct btrfs_dir_item *di;
>> + struct btrfs_key key;
>> + u64 dir_id;
>> + int ret;
>> +
>> + path = btrfs_alloc_path();
>> + if (!path)
>> + return -ENOMEM;
>> +
>> + /* Make sure this root isn't set as the default subvol */
>> + dir_id = btrfs_super_root_dir(fs_info->super_copy);
>> + di = btrfs_lookup_dir_item(NULL, fs_info->tree_root, path,
>> + dir_id, "default", 7, 0);
>> + if (di && !IS_ERR(di)) {
>> + btrfs_dir_item_key_to_cpu(path->nodes[0], di, &key);
>> + if (key.objectid == root->root_key.objectid) {
>> + ret = -EPERM;
>> + btrfs_err(fs_info,
>> + "deleting default subvolume %llu is not allowed",
>> + key.objectid);
>> + goto out;
>> + }
>> + btrfs_release_path(path);
>> + }
>> +
>> + key.objectid = root->root_key.objectid;
>> + key.type = BTRFS_ROOT_REF_KEY;
>> + key.offset = (u64)-1;
>> +
>> + ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
>> + if (ret < 0)
>> + goto out;
>> + BUG_ON(ret == 0);
>> +
>> + ret = 0;
>> + if (path->slots[0] > 0) {
>> + path->slots[0]--;
>> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> + if (key.objectid == root->root_key.objectid &&
>> + key.type == BTRFS_ROOT_REF_KEY)
>> + ret = -ENOTEMPTY;
>> + }
>> +out:
>> + btrfs_free_path(path);
>> + return ret;
>> +}
>> +
>> +int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
>> +{
>> + struct btrfs_fs_info *fs_info = btrfs_sb(dentry->d_sb);
>> + struct btrfs_root *root = BTRFS_I(dir)->root;
>> + struct inode *inode = d_inode(dentry);
>> + struct btrfs_root *dest = BTRFS_I(inode)->root;
>> + struct btrfs_trans_handle *trans;
>> + struct btrfs_block_rsv block_rsv;
>> + u64 root_flags;
>> + u64 qgroup_reserved;
>> + int ret;
>> + int err;
>> +
>> + /*
>> + * Don't allow to delete a subvolume with send in progress. This is
>> + * inside the i_mutex so the error handling that has to drop the bit
>> + * again is not run concurrently.
>> + */
>> + spin_lock(&dest->root_item_lock);
>> + root_flags = btrfs_root_flags(&dest->root_item);
>> + if (dest->send_in_progress == 0) {
>> + btrfs_set_root_flags(&dest->root_item,
>> + root_flags | BTRFS_ROOT_SUBVOL_DEAD);
>> + spin_unlock(&dest->root_item_lock);
>> + } else {
>> + spin_unlock(&dest->root_item_lock);
>> + btrfs_warn(fs_info,
>> + "Attempt to delete subvolume %llu during send",
>> + dest->root_key.objectid);
>> + err = -EPERM;
>> + return err;
>> + }
>> +
>> + down_write(&fs_info->subvol_sem);
>> +
>> + err = may_destroy_subvol(dest);
>> + if (err)
>> + goto out_up_write;
>> +
>> + btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
>> + /*
>> + * One for dir inode, two for dir entries, two for root
>> + * ref/backref.
>> + */
>> + err = btrfs_subvolume_reserve_metadata(root, &block_rsv,
>> + 5, &qgroup_reserved, true);
>> + if (err)
>> + goto out_up_write;
>> +
>> + trans = btrfs_start_transaction(root, 0);
>> + if (IS_ERR(trans)) {
>> + err = PTR_ERR(trans);
>> + goto out_release;
>> + }
>> + trans->block_rsv = &block_rsv;
>> + trans->bytes_reserved = block_rsv.size;
>> +
>> + btrfs_record_snapshot_destroy(trans, BTRFS_I(dir));
>> +
>> + ret = btrfs_unlink_subvol(trans, root, dir,
>> + dest->root_key.objectid,
>> + dentry->d_name.name,
>> + dentry->d_name.len);
>> + if (ret) {
>> + err = ret;
>> + btrfs_abort_transaction(trans, ret);
>> + goto out_end_trans;
>> + }
>> +
>> + btrfs_record_root_in_trans(trans, dest);
>> +
>> + memset(&dest->root_item.drop_progress, 0,
>> + sizeof(dest->root_item.drop_progress));
>> + dest->root_item.drop_level = 0;
>> + btrfs_set_root_refs(&dest->root_item, 0);
>> +
>> + if (!test_and_set_bit(BTRFS_ROOT_ORPHAN_ITEM_INSERTED, &dest->state)) {
>> + ret = btrfs_insert_orphan_item(trans,
>> + fs_info->tree_root,
>> + dest->root_key.objectid);
>> + if (ret) {
>> + btrfs_abort_transaction(trans, ret);
>> + err = ret;
>> + goto out_end_trans;
>> + }
>> + }
>> +
>> + ret = btrfs_uuid_tree_rem(trans, fs_info, dest->root_item.uuid,
>> + BTRFS_UUID_KEY_SUBVOL,
>> + dest->root_key.objectid);
>> + if (ret && ret != -ENOENT) {
>> + btrfs_abort_transaction(trans, ret);
>> + err = ret;
>> + goto out_end_trans;
>> + }
>> + if (!btrfs_is_empty_uuid(dest->root_item.received_uuid)) {
>> + ret = btrfs_uuid_tree_rem(trans, fs_info,
>> + dest->root_item.received_uuid,
>> + BTRFS_UUID_KEY_RECEIVED_SUBVOL,
>> + dest->root_key.objectid);
>> + if (ret && ret != -ENOENT) {
>> + btrfs_abort_transaction(trans, ret);
>> + err = ret;
>> + goto out_end_trans;
>> + }
>> + }
>> +
>> +out_end_trans:
>> + trans->block_rsv = NULL;
>> + trans->bytes_reserved = 0;
>> + ret = btrfs_end_transaction(trans);
>> + if (ret && !err)
>> + err = ret;
>> + inode->i_flags |= S_DEAD;
>> +out_release:
>> + btrfs_subvolume_release_metadata(fs_info, &block_rsv);
>> +out_up_write:
>> + up_write(&fs_info->subvol_sem);
>> + if (err) {
>> + spin_lock(&dest->root_item_lock);
>> + root_flags = btrfs_root_flags(&dest->root_item);
>> + btrfs_set_root_flags(&dest->root_item,
>> + root_flags & ~BTRFS_ROOT_SUBVOL_DEAD);
>> + spin_unlock(&dest->root_item_lock);
>> + }
>> +
>> + if (!err) {
>> + d_invalidate(dentry);
>> + btrfs_invalidate_inodes(dest);
>> + ASSERT(dest->send_in_progress == 0);
>> +
>> + /* the last ref */
>> + if (dest->ino_cache_inode) {
>> + iput(dest->ino_cache_inode);
>> + dest->ino_cache_inode = NULL;
>> + }
>> + }
>> +
>> + return err;
>> +}
>> +
>> static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
>> {
>> struct inode *inode = d_inode(dentry);
>> @@ -4344,7 +4539,7 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
>> if (inode->i_size > BTRFS_EMPTY_DIR_SIZE)
>> return -ENOTEMPTY;
>> if (btrfs_ino(BTRFS_I(inode)) == BTRFS_FIRST_FREE_OBJECTID)
>> - return -EPERM;
>> + return btrfs_delete_subvolume(dir, dentry);
>>
>> trans = __unlink_start_trans(dir);
>> if (IS_ERR(trans))
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 111ee282b777..7559a1a82e6d 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1843,60 +1843,6 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>> return ret;
>> }
>>
>> -/*
>> - * helper to check if the subvolume references other subvolumes
>> - */
>> -static noinline int may_destroy_subvol(struct btrfs_root *root)
>> -{
>> - struct btrfs_fs_info *fs_info = root->fs_info;
>> - struct btrfs_path *path;
>> - struct btrfs_dir_item *di;
>> - struct btrfs_key key;
>> - u64 dir_id;
>> - int ret;
>> -
>> - path = btrfs_alloc_path();
>> - if (!path)
>> - return -ENOMEM;
>> -
>> - /* Make sure this root isn't set as the default subvol */
>> - dir_id = btrfs_super_root_dir(fs_info->super_copy);
>> - di = btrfs_lookup_dir_item(NULL, fs_info->tree_root, path,
>> - dir_id, "default", 7, 0);
>> - if (di && !IS_ERR(di)) {
>> - btrfs_dir_item_key_to_cpu(path->nodes[0], di, &key);
>> - if (key.objectid == root->root_key.objectid) {
>> - ret = -EPERM;
>> - btrfs_err(fs_info,
>> - "deleting default subvolume %llu is not allowed",
>> - key.objectid);
>> - goto out;
>> - }
>> - btrfs_release_path(path);
>> - }
>> -
>> - key.objectid = root->root_key.objectid;
>> - key.type = BTRFS_ROOT_REF_KEY;
>> - key.offset = (u64)-1;
>> -
>> - ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
>> - if (ret < 0)
>> - goto out;
>> - BUG_ON(ret == 0);
>> -
>> - ret = 0;
>> - if (path->slots[0] > 0) {
>> - path->slots[0]--;
>> - btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> - if (key.objectid == root->root_key.objectid &&
>> - key.type == BTRFS_ROOT_REF_KEY)
>> - ret = -ENOTEMPTY;
>> - }
>> -out:
>> - btrfs_free_path(path);
>> - return ret;
>> -}
>> -
>> static noinline int key_in_sk(struct btrfs_key *key,
>> struct btrfs_ioctl_search_key *sk)
>> {
>> @@ -2320,12 +2266,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>> struct btrfs_root *root = BTRFS_I(dir)->root;
>> struct btrfs_root *dest = NULL;
>> struct btrfs_ioctl_vol_args *vol_args;
>> - struct btrfs_trans_handle *trans;
>> - struct btrfs_block_rsv block_rsv;
>> - u64 root_flags;
>> - u64 qgroup_reserved;
>> int namelen;
>> - int ret;
>> int err = 0;
>>
>> if (!S_ISDIR(dir->i_mode))
>> @@ -2409,133 +2350,11 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>> }
>>
>> inode_lock(inode);
>> -
>> - /*
>> - * Don't allow to delete a subvolume with send in progress. This is
>> - * inside the i_mutex so the error handling that has to drop the bit
>> - * again is not run concurrently.
>> - */
>> - spin_lock(&dest->root_item_lock);
>> - root_flags = btrfs_root_flags(&dest->root_item);
>> - if (dest->send_in_progress == 0) {
>> - btrfs_set_root_flags(&dest->root_item,
>> - root_flags | BTRFS_ROOT_SUBVOL_DEAD);
>> - spin_unlock(&dest->root_item_lock);
>> - } else {
>> - spin_unlock(&dest->root_item_lock);
>> - btrfs_warn(fs_info,
>> - "Attempt to delete subvolume %llu during send",
>> - dest->root_key.objectid);
>> - err = -EPERM;
>> - goto out_unlock_inode;
>> - }
>> -
>> - down_write(&fs_info->subvol_sem);
>> -
>> - err = may_destroy_subvol(dest);
>> - if (err)
>> - goto out_up_write;
>> -
>> - btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
>> - /*
>> - * One for dir inode, two for dir entries, two for root
>> - * ref/backref.
>> - */
>> - err = btrfs_subvolume_reserve_metadata(root, &block_rsv,
>> - 5, &qgroup_reserved, true);
>> - if (err)
>> - goto out_up_write;
>> -
>> - trans = btrfs_start_transaction(root, 0);
>> - if (IS_ERR(trans)) {
>> - err = PTR_ERR(trans);
>> - goto out_release;
>> - }
>> - trans->block_rsv = &block_rsv;
>> - trans->bytes_reserved = block_rsv.size;
>> -
>> - btrfs_record_snapshot_destroy(trans, BTRFS_I(dir));
>> -
>> - ret = btrfs_unlink_subvol(trans, root, dir,
>> - dest->root_key.objectid,
>> - dentry->d_name.name,
>> - dentry->d_name.len);
>> - if (ret) {
>> - err = ret;
>> - btrfs_abort_transaction(trans, ret);
>> - goto out_end_trans;
>> - }
>> -
>> - btrfs_record_root_in_trans(trans, dest);
>> -
>> - memset(&dest->root_item.drop_progress, 0,
>> - sizeof(dest->root_item.drop_progress));
>> - dest->root_item.drop_level = 0;
>> - btrfs_set_root_refs(&dest->root_item, 0);
>> -
>> - if (!test_and_set_bit(BTRFS_ROOT_ORPHAN_ITEM_INSERTED, &dest->state)) {
>> - ret = btrfs_insert_orphan_item(trans,
>> - fs_info->tree_root,
>> - dest->root_key.objectid);
>> - if (ret) {
>> - btrfs_abort_transaction(trans, ret);
>> - err = ret;
>> - goto out_end_trans;
>> - }
>> - }
>> -
>> - ret = btrfs_uuid_tree_rem(trans, fs_info, dest->root_item.uuid,
>> - BTRFS_UUID_KEY_SUBVOL,
>> - dest->root_key.objectid);
>> - if (ret && ret != -ENOENT) {
>> - btrfs_abort_transaction(trans, ret);
>> - err = ret;
>> - goto out_end_trans;
>> - }
>> - if (!btrfs_is_empty_uuid(dest->root_item.received_uuid)) {
>> - ret = btrfs_uuid_tree_rem(trans, fs_info,
>> - dest->root_item.received_uuid,
>> - BTRFS_UUID_KEY_RECEIVED_SUBVOL,
>> - dest->root_key.objectid);
>> - if (ret && ret != -ENOENT) {
>> - btrfs_abort_transaction(trans, ret);
>> - err = ret;
>> - goto out_end_trans;
>> - }
>> - }
>> -
>> -out_end_trans:
>> - trans->block_rsv = NULL;
>> - trans->bytes_reserved = 0;
>> - ret = btrfs_end_transaction(trans);
>> - if (ret && !err)
>> - err = ret;
>> - inode->i_flags |= S_DEAD;
>> -out_release:
>> - btrfs_subvolume_release_metadata(fs_info, &block_rsv);
>> -out_up_write:
>> - up_write(&fs_info->subvol_sem);
>> - if (err) {
>> - spin_lock(&dest->root_item_lock);
>> - root_flags = btrfs_root_flags(&dest->root_item);
>> - btrfs_set_root_flags(&dest->root_item,
>> - root_flags & ~BTRFS_ROOT_SUBVOL_DEAD);
>> - spin_unlock(&dest->root_item_lock);
>> - }
>> -out_unlock_inode:
>> + err = btrfs_delete_subvolume(dir, dentry);
>> inode_unlock(inode);
>> - if (!err) {
>> - d_invalidate(dentry);
>> - btrfs_invalidate_inodes(dest);
>> + if (!err)
>> d_delete(dentry);
>> - ASSERT(dest->send_in_progress == 0);
>>
>> - /* the last ref */
>> - if (dest->ino_cache_inode) {
>> - iput(dest->ino_cache_inode);
>> - dest->ino_cache_inode = NULL;
>> - }
>> - }
>> out_dput:
>> dput(dentry);
>> out_unlock_dir:
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
