[PATCH] btrfs-progs: tests: test mkfs.btrfs fails on small backing size thin provision device
This tests is most similar to xfstests generic/405. It calls device mapper to create a thin provision device with small backing size and big virtual size. mkfs.btrfs should fail on such devices. This test should pass after commit e805b143a4fe ("btrfs-progs: mkfs: return nozero value on thin provisioned device"). Signed-off-by: Su Yue --- .../test.sh | 93 +++ 1 file changed, 93 insertions(+) create mode 100755 tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh diff --git a/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh new file mode 100755 index ..f2e044da5d17 --- /dev/null +++ b/tests/mkfs-tests/017-small-backing-size-thin-provision-device/test.sh @@ -0,0 +1,93 @@ +#!/bin/bash +# mkfs.btrfs must failed on a thin provision device with very small +# backing size and big virtual size. + +source "$TEST_TOP/common" + +check_prereq mkfs.btrfs + +setup_root_helper +prepare_test_dev + +# Backing data dev +DMTHIN_DATA_NAME="thin-data" +DMTHIN_DATA_DEV="/dev/mapper/$DMTHIN_DATA_NAME" +# Backing metadata dev +DMTHIN_META_NAME="thin-meta" +DMTHIN_META_DEV="/dev/mapper/$DMTHIN_META_NAME" +# Backing pool dev (combination of above) +DMTHIN_POOL_NAME="thin-pool" +DMTHIN_POOL_DEV="/dev/mapper/$DMTHIN_POOL_NAME" +# Thin volume +DMTHIN_VOL_NAME="thin-vol" +DMTHIN_VOL_DEV="/dev/mapper/$DMTHIN_VOL_NAME" + +dmthin_cleanup() +{ + # wait for device to be fully settled + run_check $SUDO_HELPER udevadm settle + run_check $SUDO_HELPER dmsetup remove $DMTHIN_VOL_NAME + run_check $SUDO_HELPER dmsetup remove $DMTHIN_POOL_NAME + run_check $SUDO_HELPER dmsetup remove $DMTHIN_META_NAME + run_check $SUDO_HELPER dmsetup remove $DMTHIN_DATA_NAME +} + +sector_size=512 # in bytes +data_dev_size=$((1 * 1024 * 1024 / $sector_size)) # 1M +virtual_size=$((1 * 1024 * 1024 * 1024 * 1024 / $sector_size)) # 1T +cluster_size=1024# 512k in sectors +low_water=$((104857600 / $cluster_size/ $sector_size)) # 100M / $cluster_size, in sectors + +# Need to make linear metadata and data devs. From kernel docs: +# As a guide, we suggest you calculate the number of bytes to use in the +# metadata device as 48 * $data_dev_size / $data_block_size but round it up +# to 2MB (4096 sectors) if the answer is smaller. +# So do that: +meta_dev_size=$((48 * $data_dev_size / $cluster_size)) +if [ "$meta_dev_size" -lt "4096" ]; then +meta_dev_size=4096 # 2MB +fi + +meta_dev_offset=0 +total_data_dev_size=$(($meta_dev_offset + $meta_dev_size + $data_dev_size)) + +run_check truncate -s0 img +chmod a+w img +run_check truncate -s"$(($total_data_dev_size * $sector_size))" img + +dm_backing_dev=`run_check_stdout $SUDO_HELPER losetup --find --show img` + +# Metadata device +DMTHIN_META_TABLE="0 $meta_dev_size linear $dm_backing_dev $meta_dev_offset" +run_check $SUDO_HELPER dmsetup create $DMTHIN_META_NAME --table "$DMTHIN_META_TABLE" + +# Data device +data_dev_offset=$((meta_dev_offset + $meta_dev_size)) +DMTHIN_DATA_TABLE="0 $data_dev_size linear $dm_backing_dev $data_dev_offset" +run_check $SUDO_HELPER dmsetup create $DMTHIN_DATA_NAME --table "$DMTHIN_DATA_TABLE" + +# Zap the pool metadata dev +run_check dd if=/dev/zero of=$DMTHIN_META_DEV bs=4096 count=1 + +# Thin pool +# "start length thin-pool metadata_dev data_dev data_block_size low_water_mark" +DMTHIN_POOL_TABLE="0 $data_dev_size thin-pool $DMTHIN_META_DEV $DMTHIN_DATA_DEV $cluster_size $low_water" +run_check $SUDO_HELPER dmsetup create $DMTHIN_POOL_NAME --table "$DMTHIN_POOL_TABLE" + +# Thin volume +pool_id=$RANDOM +run_check $SUDO_HELPER dmsetup message $DMTHIN_POOL_DEV 0 "create_thin $pool_id" + +# start length thin pool_dev dev_id [external_origin_dev] +DMTHIN_VOL_TABLE="0 $virtual_size thin $DMTHIN_POOL_DEV $pool_id" +run_check $SUDO_HELPER dmsetup create $DMTHIN_VOL_NAME --table "$DMTHIN_VOL_TABLE" + +# mkfs.btrfs should fail due to the small backing device +run_mustfail "should fail for samll backing size thin provision device" \ +$SUDO_HELPER "$TOP/mkfs.btrfs" -f "$@" "$DMTHIN_VOL_DEV" + +#cleanup +dmthin_cleanup +run_mayfail $SUDO_HELPER losetup -d $dm_backing_dev +run_check truncate -s0 img +rm img -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] btrfs-progs: remove meaningless process
Variable do_wait is synchronized with the variable do_background, when if(do_background) is true, if(!do_wait) is also true, so parent process will goto out immediately. The following wait never be run. And if option -B is chosen, when do_background is 0 and do_wait is 1, there is no need to fork a child process to wait for scrub over. So remove unnecessary process. Since parent process go out immediately, so remove SIGINT process of parent process too. Changelog: v2: remove the SIGINT process of parent process. v1: remove do_wait variable and wait process Signed-off-by: Gu Jinxiang --- cmds-scrub.c | 30 -- 1 file changed, 30 deletions(-) diff --git a/cmds-scrub.c b/cmds-scrub.c index efd7db94..c5f908f4 100644 --- a/cmds-scrub.c +++ b/cmds-scrub.c @@ -312,16 +312,6 @@ static void scrub_sigint_record_progress(int signal) perror("Scrub cancel failed"); } -static int scrub_handle_sigint_parent(void) -{ - struct sigaction sa = { - .sa_handler = SIG_IGN, - .sa_flags = SA_RESTART, - }; - - return sigaction(SIGINT, &sa, NULL); -} - static int scrub_handle_sigint_child(int fd) { struct sigaction sa = { @@ -1109,7 +1099,6 @@ static int scrub_start(int argc, char **argv, int resume) int print_raw = 0; char *path; int do_background = 1; - int do_wait = 0; int do_print = 0; int do_quiet = 0; int do_record = 1; @@ -1147,7 +1136,6 @@ static int scrub_start(int argc, char **argv, int resume) switch (c) { case 'B': do_background = 0; - do_wait = 1; do_print = 1; break; case 'd': @@ -1374,28 +1362,10 @@ static int scrub_start(int argc, char **argv, int resume) } if (pid) { - int stat; - scrub_handle_sigint_parent(); if (!do_quiet) printf("scrub %s on %s, fsid %s (pid=%d)\n", n_start ? "started" : "resumed", path, fsid, pid); - if (!do_wait) { - err = 0; - goto out; - } - ret = wait(&stat); - if (ret != pid) { - error_on(!do_quiet, "wait failed (ret=%d): %m", - ret); - err = 1; - goto out; - } - if (!WIFEXITED(stat) || WEXITSTATUS(stat)) { - error_on(!do_quiet, "scrub process failed"); - err = WIFEXITED(stat) ? WEXITSTATUS(stat) : -1; - goto out; - } err = 0; goto out; } -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] btrfs-progs: remove meaningless process
Variable do_wait is synchronized with the variable do_background, when if(do_background) is true, if(!do_wait) is also true, so parent process will goto out immediately. The following wait never be run. And if option -B is chosen, when do_background is 0 and do_wait is 1, there is no need to fork a child process to wait for scrub over. So remove unnecessary process. Signed-off-by: Gu Jinxiang --- cmds-scrub.c | 19 --- 1 file changed, 19 deletions(-) diff --git a/cmds-scrub.c b/cmds-scrub.c index efd7db94..3fb16887 100644 --- a/cmds-scrub.c +++ b/cmds-scrub.c @@ -1109,7 +1109,6 @@ static int scrub_start(int argc, char **argv, int resume) int print_raw = 0; char *path; int do_background = 1; - int do_wait = 0; int do_print = 0; int do_quiet = 0; int do_record = 1; @@ -1147,7 +1146,6 @@ static int scrub_start(int argc, char **argv, int resume) switch (c) { case 'B': do_background = 0; - do_wait = 1; do_print = 1; break; case 'd': @@ -1374,28 +1372,11 @@ static int scrub_start(int argc, char **argv, int resume) } if (pid) { - int stat; scrub_handle_sigint_parent(); if (!do_quiet) printf("scrub %s on %s, fsid %s (pid=%d)\n", n_start ? "started" : "resumed", path, fsid, pid); - if (!do_wait) { - err = 0; - goto out; - } - ret = wait(&stat); - if (ret != pid) { - error_on(!do_quiet, "wait failed (ret=%d): %m", - ret); - err = 1; - goto out; - } - if (!WIFEXITED(stat) || WEXITSTATUS(stat)) { - error_on(!do_quiet, "scrub process failed"); - err = WIFEXITED(stat) ? WEXITSTATUS(stat) : -1; - goto out; - } err = 0; goto out; } -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/4] btrfs: Allow rmdir(2) to delete an empty subvolume
Change the behaior of rmdir(2) and allow it to delete an empty subvolume by calling btrfs_delete_subvolume() which is used by btrfs_ioctl_snap_destroy(). The required lock for @dir and inode of @dentry is already acquired in vfs layer. We need some check before deleting a subvolume. Permission check is done in vfs layer, emptiness check is in btrfs_rmdir() and additional check (i.e. neither send is in progress nor the subvolume is a default subvolume) is in btrfs_delete_subvolume(). Note that in btrfs_snap_destroy(), d_delete() is called after btrfs_delete_subvolume(). For rmdir(2), d_delete() is called in vfs layer later. Tested-by: Goffredo Baroncelli Signed-off-by: Tomohiro Misono --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 2db2e860ba90..afc631f28088 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4537,7 +4537,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)) -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/4] btrfs: Refactor btrfs_ioctl_snap_destroy() by using btrfs_delete_subvolume()
Use btrfs_delete_subvolume() to refactor btrfs_ioctl_snap_destroy(). The permission check is still done in btrfs_ioctl_snap_destroy(). Also, call of d_delete() is still required since btrfs_delete_subvolume() does not call it. As a result, btrfs_unlink_subvol() and may_destroy_subvol() become static functions. No functional change happens. Signed-off-by: Tomohiro Misono --- fs/btrfs/ctree.h | 5 --- fs/btrfs/inode.c | 4 +- fs/btrfs/ioctl.c | 131 +-- 3 files changed, 4 insertions(+), 136 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 618365eb9d84..6f23f0694ac3 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3162,11 +3162,6 @@ 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); -noinline int may_destroy_subvol(struct btrfs_root *root); 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); diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 54c50a9fa2ee..2db2e860ba90 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) @@ -4336,7 +4336,7 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans, /* * helper to check if the subvolume references other subvolumes */ -noinline int may_destroy_subvol(struct btrfs_root *root) +static noinline int may_destroy_subvol(struct btrfs_root *root) { struct btrfs_fs_info *fs_info = root->fs_info; struct btrfs_path *path; diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 11af9eb449ef..7559a1a82e6d 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2266,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)) @@ -2355,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) { -
[PATCH v4 2/4] btrfs: Add definition of btrfs_delete_subvolume()
Add new function btrfs_delete_subvolume() which is almost identical to the secand half of btrfs_ioctl_snap_destroy(). This function requires inode_lock for both @dir and inode of @dentry. This performes additional check (1. send is not in progress. 2. the subvolume is not a default subvolume. 3. the subvolume does not contain other subvoume) and actual deletion process. The code path is not changed yet and this function will be used to refactor btrfs_ioctl_snap_destroy() and to allow rmdir(2) to delete an empty subvolume. Signed-off-by: Tomohiro Misono --- fs/btrfs/ctree.h | 1 + fs/btrfs/inode.c | 139 +++ 2 files changed, 140 insertions(+) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 2dbead106aab..618365eb9d84 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3167,6 +3167,7 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans, struct inode *dir, u64 objectid, const char *name, int name_len); noinline int may_destroy_subvol(struct btrfs_root *root); +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 db66fa4fede6..54c50a9fa2ee 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4387,6 +4387,145 @@ noinline int may_destroy_subvol(struct btrfs_root *root) 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 != -ENOEN
[PATCH v4 1/4] btrfs: move may_destroy_subvol() from ioctl.c to inode.c
This is a preparation work to refactor btrfs_ioctl_snap_destroy() and to allow rmdir(2) to delete an empty subvolume. Signed-off-by: Tomohiro Misono --- fs/btrfs/ctree.h | 1 + fs/btrfs/inode.c | 54 ++ fs/btrfs/ioctl.c | 54 -- 3 files changed, 55 insertions(+), 54 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index da308774b8a4..2dbead106aab 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3166,6 +3166,7 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans, struct btrfs_root *root, struct inode *dir, u64 objectid, const char *name, int name_len); +noinline int may_destroy_subvol(struct btrfs_root *root); 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..db66fa4fede6 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4333,6 +4333,60 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans, return ret; } +/* + * helper to check if the subvolume references other subvolumes + */ +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 int btrfs_rmdir(struct inode *dir, struct dentry *dentry) { struct inode *inode = d_inode(dentry); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 111ee282b777..11af9eb449ef 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], &k
[PATCH v4 0/4] Allow rmdir(2) to delete a subvolume
changelog: v3 -> v4 ... Reorganize patches and updates commit log. No code change in total v2 -> v3 ... Use if-else block instead of two if blocks and add Tested-by tag in 2nd patch v1 -> v2 ... Split the patch to hopefully make review easier Note: I will send a xfstest if this series is merged. This series changes the behavior of rmdir(2) and allow it to delete an empty subvolume. 1st to 3rd patches refactor btrfs_ioctl_snap_destroy() and no functional change happens. btrfs_ioctl_snap_destroy() is roughly comprised of two parts. The first part is permission check of the (parent) subvolume. The second part is additional check (1. send is not in progress. 2. the subvolume is not a default subvolume. 3. the subvolume does not contain other subvoume) and actual deletion process. The aim of 1st to 3rd patches is to extract the second part as btrfs_delete_subvolume(). 4th patch is just one line but this changes the behavior of rmdir(2) by calling btrfs_delete_subvolume(). For rmdir(2), permission check is already done in vfs layer. Tomohiro Misono (4): btrfs: move may_destroy_subvol() from ioctl.c to inode.c btrfs: Add definition of btrfs_delete_subvolume() btrfs: Refactor btrfs_ioctl_snap_destroy() by using btrfs_delete_subvolume() btrfs: Allow rmdir(2) to delete an empty subvolume fs/btrfs/ctree.h | 5 +- fs/btrfs/inode.c | 197 ++- fs/btrfs/ioctl.c | 185 +-- 3 files changed, 198 insertions(+), 189 deletions(-) -- 2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] btrfs: Allow rmdir(2) to delete a subvolume
On 2018/04/06 23:46, David Sterba wrote: > On Fri, Mar 30, 2018 at 03:16:47PM +0900, Misono Tomohiro wrote: >> This patch changes the behavior of rmdir(2) to allow it to delete >> an empty subvolume by default, unless it is not a default subvolume >> and send is not in progress. >> >> New function btrfs_delete_subvolume() is almost equal to the second half >> of btrfs_ioctl_snap_destroy(). This function requires inode_lock for both >> @dir and inode of @dentry. For rmdir(2) it is already acquired in vfs >> layer before calling btrfs_rmdir(). >> >> Note that while a non-privileged user cannot delete a read-only subvolume >> by "btrfs subvolume delete" when user_subvol_rm_allowd mount option is >> enabled, rmdir(2) can delete an empty read-only subvolume. >> >> Tested-by: Goffredo Baroncelli >> Signed-off-by: Tomohiro Misono >> --- >> fs/btrfs/inode.c | 141 >> ++- >> 1 file changed, 140 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index db66fa4fede6..84dbb9cafd6b 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -4387,6 +4387,145 @@ noinline int may_destroy_subvol(struct btrfs_root >> *root) >> return ret; >> } >> >> +static 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, >> +
Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
On 2018年04月11日 05:04, Goffredo Baroncelli wrote: > Hi Qu, > > On 04/10/2018 04:00 AM, Qu Wenruo wrote: >> >> >> On 2018年04月10日 05:50, Goffredo Baroncelli wrote: >>> Hi Qu, >>> >>> On 04/09/2018 11:19 AM, Qu Wenruo wrote: When manually patching super blocks, current validation check is pretty weak (limited to magic number and csum) and doesn't provide extra check for some obvious corruption (like invalid sectorsize). >>> [...] Signed-off-by: Qu Wenruo --- changelog: v2: Generate tab based indent string in helper macro instead of passing spacer string from outside, suggested by Nikolay. (In fact, if using %*s it would be much easier, however it needs extra rework for existing code as they still use tab as indent) --- cmds-inspect-dump-super.c | 206 +- 1 file changed, 137 insertions(+), 69 deletions(-) diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c index 24588c87cce6..68d6f59ad727 100644 --- a/cmds-inspect-dump-super.c +++ b/cmds-inspect-dump-super.c @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag) super_flags_num, BTRFS_SUPER_FLAG_SUPP); } +#define INDENT_BUFFER_LEN 80 +#define TAB_LEN 8 +#define SUPER_INDENT 24 + +/* helper to generate tab based indent string */ +static void generate_tab_indent(char *buf, unsigned int indent) +{ + buf[0] = '\0'; + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN) + strncat(buf, "\t", INDENT_BUFFER_LEN); +} + +/* Helper to print member in %llu */ +#define print_super(sb, member) \ +({ \ + int indent = SUPER_INDENT - strlen(#member);\ + char indent_str[INDENT_BUFFER_LEN]; \ + \ + generate_tab_indent(indent_str, indent);\ + printf("%s%s%llu\n", #member, indent_str, \ + (u64) btrfs_super_##member(sb)); \ >>> >>> Why not something like: >>> >>> static const char tabs[] = "\t\t\t\t";\ >>> int i = (sizeof(#member)+6)/8;\ >>> if (i > sizeof(tabs)-1) \ >>> i = sizeof(tabs-1); \ >>> u64 value = (u64)btrfs_super_##member(sb);\ >>> printf("%s%s" format, \ >>> #member, tabs+i, value); >>> >>> >>> so to get rid of generate_tab_indent and indent_str >> >> And we need to call such functions in each helper macros, with >> duplicated codes. > > Please look at the asm generated: even if the "source generated" by the > expansion of the macro is bigger, the binary code is smaller. > E.g. the code below No, I don't mean asm code, but C code. We should reduce duplicated code, as when it get modified we need to modify all places, other than just one function. For the generated asm code, it's compiler's work to optimize its size or speed, not developer. > > static const char tabs[] = "\t\t\t\t";\ > int i = (sizeof(#member)+6)/8;\ > if (i > sizeof(tabs)-1) \ > i = sizeof(tabs)-1; \ > > is translate as single move (see https://godbolt.org/g/4oxmAZ) > > main::tabs: > .string "\t\t\t\t" > > movlmain::tabs+1, %edx > > > These days, the compilers are smart enough to evaluate the code above at > compile time. This is not true for generate_tab_indent(), which is too > complex. > > Of course all the above consideration are meaningless, because I don't think > that the size (or the speed) matters in the specific case of > dump_superblock(). Exactly. > However I want to point out that the compiler sometime are smarter than the > programmer (or almost smarter than me :-) ) in a surprising way, and what > would seems bigger at the end results smaller. That's why our developer should care about C code duplications other than generated asm code size. > > [...] > + */ +#define print_check_super(sb, member, bad_condition) \ +({ \ + int indent = SUPER_INDENT - strlen(#member);\ + char indent_str[INDENT_BUFFER_LEN]; \ + u64 value; \ +
Funds Plans..
Hello, My name is Hon. Ms. Reem Al-Hashimy I am the present Ministry of petroleum in UAE. I write to solicit for your partnership in claiming of $47 Million USD from a Financial Home in Uk. The aforementioned fund $47 Million USD is my share percentage from a Oil companies that I helped financed, influentially but I don't want my government to know about the fund. Furthermore, as a Ministry of Mining, I am not allowed to be part of such a deal, because it's against my country professional practice policy. So I am compelled to ask that you stand on my behalf and receive this fund into any account that is solely controlled by you. I will compensate you with 40% of the total amount involved as gratification for being my partner in the transfer. Please reply to my private email: (maithasa...@minister.com) Regards, Ms. Reem Al-Hashimy -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] btrfs-progs: dump-super: Refactor print function and add extra check
Hi Qu, On 04/10/2018 04:00 AM, Qu Wenruo wrote: > > > On 2018年04月10日 05:50, Goffredo Baroncelli wrote: >> Hi Qu, >> >> On 04/09/2018 11:19 AM, Qu Wenruo wrote: >>> When manually patching super blocks, current validation check is pretty >>> weak (limited to magic number and csum) and doesn't provide extra check >>> for some obvious corruption (like invalid sectorsize). >> [...] >>> >>> Signed-off-by: Qu Wenruo >>> --- >>> changelog: >>> v2: >>> Generate tab based indent string in helper macro instead of passing spacer >>> string from outside, suggested by Nikolay. >>> (In fact, if using %*s it would be much easier, however it needs extra >>>rework for existing code as they still use tab as indent) >>> --- >>> cmds-inspect-dump-super.c | 206 +- >>> 1 file changed, 137 insertions(+), 69 deletions(-) >>> >>> diff --git a/cmds-inspect-dump-super.c b/cmds-inspect-dump-super.c >>> index 24588c87cce6..68d6f59ad727 100644 >>> --- a/cmds-inspect-dump-super.c >>> +++ b/cmds-inspect-dump-super.c >>> @@ -312,11 +312,80 @@ static void print_readable_super_flag(u64 flag) >>> super_flags_num, BTRFS_SUPER_FLAG_SUPP); >>> } >>> >>> +#define INDENT_BUFFER_LEN 80 >>> +#define TAB_LEN8 >>> +#define SUPER_INDENT 24 >>> + >>> +/* helper to generate tab based indent string */ >>> +static void generate_tab_indent(char *buf, unsigned int indent) >>> +{ >>> + buf[0] = '\0'; >>> + for (indent = round_up(indent, TAB_LEN); indent > 0; indent -= TAB_LEN) >>> + strncat(buf, "\t", INDENT_BUFFER_LEN); >>> +} >>> + >>> +/* Helper to print member in %llu */ >>> +#define print_super(sb, member) >>> \ >>> +({ \ >>> + int indent = SUPER_INDENT - strlen(#member);\ >>> + char indent_str[INDENT_BUFFER_LEN]; \ >>> + \ >>> + generate_tab_indent(indent_str, indent);\ >>> + printf("%s%s%llu\n", #member, indent_str, \ >>> + (u64) btrfs_super_##member(sb)); \ >> >> Why not something like: >> >> static const char tabs[] = "\t\t\t\t";\ >> int i = (sizeof(#member)+6)/8;\ >> if (i > sizeof(tabs)-1) \ >> i = sizeof(tabs-1); \ >> u64 value = (u64)btrfs_super_##member(sb);\ >> printf("%s%s" format, \ >> #member, tabs+i, value); >> >> >> so to get rid of generate_tab_indent and indent_str > > And we need to call such functions in each helper macros, with > duplicated codes. Please look at the asm generated: even if the "source generated" by the expansion of the macro is bigger, the binary code is smaller. E.g. the code below static const char tabs[] = "\t\t\t\t";\ int i = (sizeof(#member)+6)/8;\ if (i > sizeof(tabs)-1) \ i = sizeof(tabs)-1; \ is translate as single move (see https://godbolt.org/g/4oxmAZ) main::tabs: .string "\t\t\t\t" movlmain::tabs+1, %edx These days, the compilers are smart enough to evaluate the code above at compile time. This is not true for generate_tab_indent(), which is too complex. Of course all the above consideration are meaningless, because I don't think that the size (or the speed) matters in the specific case of dump_superblock(). However I want to point out that the compiler sometime are smarter than the programmer (or almost smarter than me :-) ) in a surprising way, and what would seems bigger at the end results smaller. [...] >>> + */ >>> +#define print_check_super(sb, member, bad_condition) >>> \ >>> +({ \ >>> + int indent = SUPER_INDENT - strlen(#member);\ >>> + char indent_str[INDENT_BUFFER_LEN]; \ >>> + u64 value; \ >>> + \ >>> + generate_tab_indent(indent_str, indent);\ >>> + value = btrfs_super_##member(sb); \ >>> + printf("%s%s%llu", #member, indent_str, (u64) value); \ >>> + if (bad_condition) \ >>> + printf(" [INVALID]"); \ >> >> What about printing also the condition: something like >> >> +if (bad_condition) \ >> +printf(" [INVALID '%s']", #bad_condition);
[PATCH] btrfs.static: needs libbtrfsutil
Add libbtrfsutil objects to btrfs.static link command. This fixes static build failure: utils.static.o: In function `parse_qgroupid': utils.c:(.text.parse_qgroupid+0xb0): undefined reference to `btrfs_util_is_subvolume' props.static.o: In function `prop_read_only': props.c:(.text.prop_read_only+0x70): undefined reference to `btrfs_util_set_subvolume_read_only' ... Makefile:457: recipe for target 'btrfs.static' failed make[1]: *** [btrfs.static] Error 1 Signed-off-by: Baruch Siach --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 5ba76d2eee40..9b97c36a44a5 100644 --- a/Makefile +++ b/Makefile @@ -453,7 +453,7 @@ btrfs: btrfs.o $(objects) $(cmds_objects) $(libs_static) @echo "[LD] $@" $(Q)$(CC) -o $@ $^ $(LDFLAGS) $(LIBS) $(LIBS_COMP) -btrfs.static: btrfs.static.o $(static_objects) $(static_cmds_objects) $(static_libbtrfs_objects) +btrfs.static: btrfs.static.o $(static_objects) $(static_cmds_objects) $(static_libbtrfs_objects) $(libbtrfsutil_objects) @echo "[LD] $@" $(Q)$(CC) -o $@ $^ $(STATIC_LDFLAGS) $(STATIC_LIBS) $(STATIC_LIBS_COMP) -- 2.16.3 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/3] btrfs: Add three new unprivileged ioctls to allow normal users to call "sub list/show" etc.
On Mon, Mar 19, 2018 at 04:27:09PM +0900, Misono, Tomohiro wrote: > changelog: > > v2-> v3 > - fix kbuild test bot warning > v1 -> v2 > - completely reimplement 1st/2nd ioctl to have user friendly api > - various cleanup, remove unnecessary goto > === > > This adds three new unprivileged ioctls: > > 1st patch: ioctl which returns subvolume information of ROOT_ITEM and > ROOT_BACKREF > 2nd patch: ioctl which returns subvolume information of ROOT_REF (without > subvolume name) > 3rd patch: user version of ino_lookup ioctl which also peforms permission > check. The overall approach to listing subvolumes looks good. We can enumerate them, get the relations and the details. Making some sense of that in the userspace is a whole different and maybe more difficult topic. I hope we could use the opportunity to clean up the listing commandline interface and output at the same time, as there's going to be possibly some incompatibility introduced. We need to start with current usecases and how they're implemented or mis-implemented (ie. leading to confusion). The discussions I've read so far cover a good part of that. I'll review and add the kernel patches to misc-next to get some testing coverage. As this is a non-restricted ioctl full of pointers and bytes shuffled around, we will have to do an extra security-oriented review. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/8] btrfs: use iocb for __btrfs_buffered_write
On Fri, Nov 17, 2017 at 11:44:47AM -0600, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues > > Preparatory patch. It reduces the arguments to __btrfs_buffered_write > to follow buffered_write() style. > > Signed-off-by: Goldwyn Rodrigues I got pointed to this patch that it could be applied independently, so I'm adding it to btrfs queue. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] btrfs: Factor out write portion of btrfs_get_blocks_direct
On Thu, Feb 22, 2018 at 06:12:14PM +0200, Nikolay Borisov wrote: > Now that the read side is extracted into its own function, do the same > to the write side. This leaves btrfs_get_blocks_direct_write with the > sole purpose of handling common locking required. Also flip the > condition in btrfs_get_blocks_direct_write so that the write case > comes first and we check for if (Create) rather than if (!create). This > is purely subjective but I believe makes reading a bit more "linear". I subjectively agree. > No functional changes. > > Signed-off-by: Nikolay Borisov Reviewed-by: David Sterba Same comment as before, no __ and please use the single-return exit block where possible. > Both patches survived xfstests runs. I'll add that to misc-next, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] btrfs: Factor out read portion of btrfs_get_blocks_direct
On Thu, Feb 22, 2018 at 06:12:13PM +0200, Nikolay Borisov wrote: > Currently this function handles both the READ and WRITE dio cases. This > is facilitated by a bunch of 'if' statements, a goto short-circuit > statement and a very perverse aliasing of "!created"(READ) case > by setting lockstart = lockend and checking for lockstart < lockend for > detecting the write. Let's simplify this mess by extracting the > READ-only code into a separate __btrfs_get_block_direct_read function. > This is only the first step, the next one will be to factor out the > write side as well. The end goal will be to have the common locking/ > unlocking code in btrfs_get_blocks_direct and then it will call either > the read|write subvariants. No functional changes. Reviewed-by: David Sterba uh what a convoluted code to untangle. A few style notes below. > > Signed-off-by: Nikolay Borisov > --- > fs/btrfs/inode.c | 49 - > 1 file changed, 36 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 491a7397f6fa..fd99347d0c91 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -7677,6 +7677,27 @@ static struct extent_map *create_io_em(struct inode > *inode, u64 start, u64 len, > return em; > } > > + > +static int __btrfs_get_blocks_direct_read(struct extent_map *em, Please drop the "__" the function is static and there's no underscore-less version. > + struct buffer_head *bh_result, > + struct inode *inode, > + u64 start, u64 len) > +{ > + if (em->block_start == EXTENT_MAP_HOLE || > + test_bit(EXTENT_FLAG_PREALLOC, &em->flags)) > + return -ENOENT; > + > + len = min(len, em->len - (start - em->start)); > + > + bh_result->b_blocknr = (em->block_start + (start - em->start)) >> > + inode->i_blkbits; > + bh_result->b_size = len; > + bh_result->b_bdev = em->bdev; > + set_buffer_mapped(bh_result); > + > + return 0; > +} > + > static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock, > struct buffer_head *bh_result, int create) > { > @@ -7745,11 +7766,22 @@ static int btrfs_get_blocks_direct(struct inode > *inode, sector_t iblock, > goto unlock_err; > } > > - /* Just a good old fashioned hole, return */ > - if (!create && (em->block_start == EXTENT_MAP_HOLE || > - test_bit(EXTENT_FLAG_PREALLOC, &em->flags))) { > + if (!create) { > + ret = __btrfs_get_blocks_direct_read(em, bh_result, inode, > +start, len); > + /* Can be negative only if we read from a hole */ > + if (ret < 0) { > + ret = 0; > + free_extent_map(em); > + goto unlock_err; > + } > + /* > + * We need to unlock only the end area that we aren't using > + * if there is any leftover space > + */ > + free_extent_state(cached_state); > free_extent_map(em); > - goto unlock_err; > + return 0; Please add a separate label for that, the funcion uses the single exit block style (labels and one-or-two returns). > } > > /* > @@ -7761,12 +7793,6 @@ static int btrfs_get_blocks_direct(struct inode > *inode, sector_t iblock, >* just use the extent. >* >*/ > - if (!create) { > - len = min(len, em->len - (start - em->start)); > - lockstart = start + len; > - goto unlock; > - } > - > if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags) || > ((BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) && >em->block_start != EXTENT_MAP_HOLE)) { > @@ -7853,10 +7879,7 @@ static int btrfs_get_blocks_direct(struct inode > *inode, sector_t iblock, > clear_extent_bit(&BTRFS_I(inode)->io_tree, lockstart, >lockend, unlock_bits, 1, 0, >&cached_state); > - } else { > - free_extent_state(cached_state); > } > - > free_extent_map(em); > > return 0; -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/7] Superblock read and verify cleanups
On Fri, Mar 30, 2018 at 08:09:16AM +0800, Anand Jain wrote: > v3->v4: > Update changelog and signoff. > Reintroduce explicit check for '-EUCLEAN' > at Patch 2/8 and 5/8. > > v2->v3: > Squash > 4/8 btrfs: make btrfs_check_super_csum() non static > to > 6/8 btrfs: verify superblock checksum during scan > As in the individual patch mentioned > > v1->v2: > Various changes suggested by Nicokay. Thanks. For specific changes > pls ref to the patch. > > Patch 1-4/8 are preparatory patches adds cleanups and nonstatic requisites. > > Patch 5/8 makes sure that all copies of the superblock have the same fsid > when we scan the device. This is IMO too strict and the need of workaround puts the burden on the user while it's not necessary and scan or mount can continue as long as the first superblock is valid. > Patch 6/8 verifies superblock csum when we read it in the scan context. This was the main intention, as I read the discussion in the thread under the RFC patch. The semantics of scanning is not exactly defined, in the current code it's "pick any device that has the right magic and sb offset", so adding the csum check would add some validation but otherwise it's fine. All the work is left to mount and all reported errors can be examined immediatelly. Wiping the magic either manually or by wipefs will not touch the csum, so we don't change the behaviour. Reporting errors on each scan is a usability no-go. Patches 1-4 look ok, I'll have to think more about 5. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: fix loss of prealloc extents past i_size after fsync log replay
Hi, [This is an automated email] This commit has been processed because it contains a "Fixes:" tag, fixing commit: c71bf099abdd Btrfs: Avoid orphan inodes cleanup while replaying log. The bot has also determined it's probably a bug fixing patch. (score: 6.2138) The bot has tested the following trees: v4.16.1, v4.15.16, v4.14.33, v4.9.93, v4.4.127. v4.16.1: Build OK! v4.15.16: Build OK! v4.14.33: Build OK! v4.9.93: Build failed! Errors: tree-log.c:2367:24: error: ‘struct btrfs_fs_info’ has no member named ‘sectorsize’ tree-log.c:2367:24: error: ‘struct btrfs_fs_info’ has no member named ‘sectorsize’ tree-log.c:4224:13: error: ‘struct inode’ has no member named ‘flags’; did you mean ‘i_flags’? tree-log.c:4229:38: error: ‘struct inode’ has no member named ‘vfs_inode’ tree-log.c:4239:4: error: implicit declaration of function ‘refcount_inc’; did you mean ‘i_readcount_inc’? [-Werror=implicit-function-declaration] v4.4.127: Failed to apply! Possible dependencies: 0132761017e0 ("btrfs: fix string and comment grammatical issues and typos") -- Thanks, SashaN�r��yb�X��ǧv�^�){.n�+{�n�߲)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥
Re: How to zero errors
On 2018-04-10 09:08, James Courtier-Dutton wrote: Hi, I have disk that in the past had errors on it. I have fixed up the errors. btrfs scrub now reports no errors. How do I reset these counters to zero? BTRFS info (device sdc2): bdev /dev/sdc2 errs: wr 0, rd 35, flush 0, corrupt 1, gen 0 Run `btrfs device stats` with the `-z` option on the filesystem. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
How to zero errors
Hi, I have disk that in the past had errors on it. I have fixed up the errors. btrfs scrub now reports no errors. How do I reset these counters to zero? BTRFS info (device sdc2): bdev /dev/sdc2 errs: wr 0, rd 35, flush 0, corrupt 1, gen 0 Kind Regards James -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Btrfs: do not abort transaction when failing to insert hole extent
On Mon, Apr 09, 2018 at 06:23:14PM -0700, Liu Bo wrote: > >>> As maybe_insert_hole is only called by btrfs_cont_expand here, which > >>> means it's a really hole, I don't expect drop_extents would drop > >>> anything, we can remove this drop_extents and put an assert after > >>> btrfs_insert_file_extent for checking EEXIST. > >> Sounds good. > > Let me make a v2 and have a fstests run. > It turns out that the btrfs_drop_extents() here is quite necessary > since fallocate(2) has a FALLOC_FL_KEEP_SIZE option, and when that > happens, a hole extent would be appended between the EOF and fallocate > range's start, then a later truncate up would have to drop these hole > extents in order to expand with a new hole... Would it make sense to split the cases where FALLOC_FL_KEEP_SIZE is and is not used? Either passing an argument or 2 functions where one could avoid the drop. I've looked at the code only briefly, so this may be a nonsense in the end. > As I don't see a way to gracefully solve this except keeping > drop_extents(), lets drop this patch instead. Understood. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] btrfs: use fs_info for btrfs_handle_em_exist tracepoint
On Tue, Apr 10, 2018 at 10:45:39AM +0300, Nikolay Borisov wrote: > On 9.04.2018 14:58, David Sterba wrote: > > We really want to know to which filesystem the extent map events belong, > > but as it cannot be reached from the extent_map pointers, we need to > > pass it down the callchain. > > I really dislike propagating arguments solely for tracepoints purposes, > but if we don't have any other choice then I guess we should go with it. > However... > > > @@ -7092,7 +7092,7 @@ struct extent_map *btrfs_get_extent(struct > > btrfs_inode *inode, > > > > err = 0; > > write_lock(&em_tree->lock); > > - err = btrfs_add_extent_mapping(em_tree, &em, start, len); > > + err = btrfs_add_extent_mapping(fs_info, em_tree, &em, start, len); > > This function is called only here, and we know that em_tree passed > points to a struct, stored in btrfs_inode. So can't we use container_of > to get the btrfs_inode in this function, from where we can reference the > fs_info? I guess it could be a problem for tests. > > > Admittedly this feels somewhat hacky and I guess is not very > future-proof, but it's still a viable alternative. Sounds too fragile to me, from all the alternatives passing fs_info looks like the cleanest way for now. The filesystem UUID in the tracepoint is IMO an important part. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Tracepoint updates
On 9.04.2018 14:58, David Sterba wrote: > Switch the inode number type to u64, plus other cleanups and fixups. > > David Sterba (7): > btrfs: tracepoints, use correct type for inode number > btrfs: tracepoints, use %llu instead of %Lu > btrfs: tracepoints, drop unnecessary ULL casts > btrfs: tracepoints, fix whitespace in strings > btrfs: tracepoints, use extended format with UUID where possible > btrfs: tests: pass fs_info to extent_map tests > btrfs: use fs_info for btrfs_handle_em_exist tracepoint Reviewed-by: Nikolay Borisov > > fs/btrfs/extent_map.c | 6 +- > fs/btrfs/extent_map.h | 3 +- > fs/btrfs/inode.c | 2 +- > fs/btrfs/tests/extent-map-tests.c | 60 ++ > include/trace/events/btrfs.h | 225 > +++--- > 5 files changed, 158 insertions(+), 138 deletions(-) > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] btrfs: use fs_info for btrfs_handle_em_exist tracepoint
On 9.04.2018 14:58, David Sterba wrote: > We really want to know to which filesystem the extent map events belong, > but as it cannot be reached from the extent_map pointers, we need to > pass it down the callchain. I really dislike propagating arguments solely for tracepoints purposes, but if we don't have any other choice then I guess we should go with it. However... > > Signed-off-by: David Sterba > --- > fs/btrfs/extent_map.c | 6 -- > fs/btrfs/extent_map.h | 3 ++- > fs/btrfs/inode.c | 2 +- > fs/btrfs/tests/extent-map-tests.c | 8 > include/trace/events/btrfs.h | 12 +++- > 5 files changed, 18 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c > index 53a0633c6ef7..581b42d23e0d 100644 > --- a/fs/btrfs/extent_map.c > +++ b/fs/btrfs/extent_map.c > @@ -517,6 +517,7 @@ static noinline int merge_extent_mapping(struct > extent_map_tree *em_tree, > > /** > * btrfs_add_extent_mapping - add extent mapping into em_tree > + * @fs_info - used for tracepoint > * @em_tree - the extent tree into which we want to insert the extent mapping > * @em_in - extent we are inserting > * @start - start of the logical range btrfs_get_extent() is requesting > @@ -534,7 +535,8 @@ static noinline int merge_extent_mapping(struct > extent_map_tree *em_tree, > * Return 0 on success, otherwise -EEXIST. > * > */ > -int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > +int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info, > + struct extent_map_tree *em_tree, >struct extent_map **em_in, u64 start, u64 len) > { > int ret; > @@ -552,7 +554,7 @@ int btrfs_add_extent_mapping(struct extent_map_tree > *em_tree, > > existing = search_extent_mapping(em_tree, start, len); > > - trace_btrfs_handle_em_exist(existing, em, start, len); > + trace_btrfs_handle_em_exist(fs_info, existing, em, start, len); > > /* >* existing will always be non-NULL, since there must be > diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h > index f6f8ba114977..f55c8b4ef120 100644 > --- a/fs/btrfs/extent_map.h > +++ b/fs/btrfs/extent_map.h > @@ -91,6 +91,7 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 > start, u64 len, u64 gen > void clear_em_logging(struct extent_map_tree *tree, struct extent_map *em); > struct extent_map *search_extent_mapping(struct extent_map_tree *tree, >u64 start, u64 len); > -int btrfs_add_extent_mapping(struct extent_map_tree *em_tree, > +int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info, > + struct extent_map_tree *em_tree, >struct extent_map **em_in, u64 start, u64 len); > #endif > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 1f091c2358a4..18c31006865f 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -7092,7 +7092,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode > *inode, > > err = 0; > write_lock(&em_tree->lock); > - err = btrfs_add_extent_mapping(em_tree, &em, start, len); > + err = btrfs_add_extent_mapping(fs_info, em_tree, &em, start, len); This function is called only here, and we know that em_tree passed points to a struct, stored in btrfs_inode. So can't we use container_of to get the btrfs_inode in this function, from where we can reference the fs_info? I guess it could be a problem for tests. Admittedly this feels somewhat hacky and I guess is not very future-proof, but it's still a viable alternative. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html