[PATCH v2 0/3] btrfs-progs: prevent mkfs from aborting with small volume
Here are 3 patches to avoid undesired aborts of mkfs.btrfs. These are based on top of Chris's btrfs-progs.git: git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git Thanks, H.Seto Hidetoshi Seto (3): btrfs-progs: error if device for mkfs is too small btrfs-progs: error if device have no space to make primary chunks btrfs-progs: calculate available blocks on device properly ctree.h |8 + mkfs.c| 23 + volumes.c | 104 +--- 3 files changed, 129 insertions(+), 6 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
[PATCH v2 1/3] btrfs-progs: error if device for mkfs is too small
Eric pointed out that mkfs abort if specified volume is too small: # truncate --size=2m testfile # ./mkfs.btrfs testfile : SMALL VOLUME: forcing mixed metadata/data groups mkfs.btrfs: volumes.c:852: btrfs_alloc_chunk: Assertion `!(ret)' failed. Aborted (core dumped) As the first step to fix problems around there, let mkfs to report error if the size of target volume is less than the size of the first system block group, BTRFS_MKFS_SYSTEM_GROUP_SIZE (= 4MB). Reported-by: Eric Sandeen sand...@redhat.com Signed-off-by: Hidetoshi Seto seto.hideto...@jp.fujitsu.com --- mkfs.c |6 ++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/mkfs.c b/mkfs.c index b412b7e..a98fe54 100644 --- a/mkfs.c +++ b/mkfs.c @@ -1422,6 +1422,12 @@ int main(int ac, char **av) } } + /* To create the first block group and chunk 0 in make_btrfs */ + if (dev_block_count BTRFS_MKFS_SYSTEM_GROUP_SIZE) { + fprintf(stderr, device is too small to make filesystem\n); + exit(1); + } + blocks[0] = BTRFS_SUPER_INFO_OFFSET; for (i = 1; i 7; i++) { blocks[i] = BTRFS_SUPER_INFO_OFFSET + 1024 * 1024 + -- 1.7.1 -- 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 2/3] btrfs-progs: error if device have no space to make primary chunks
The previous patch works fine if the size of specified volume to mkfs is less than 4MB. However usually btrfs requires more than 4MB to work, and the minimum preferred size is depending on the raid setting etc. This patch let mkfs print error message if it cannot allocate one of chunks should be there at first. [before] # truncate --size=4500K testfile # ./mkfs.btrfs -f testfile : SMALL VOLUME: forcing mixed metadata/data groups mkfs.btrfs: mkfs.c:84: make_root_dir: Assertion `!(ret)' failed. Aborted (core dumped) [After] # truncate --size=4500K testfile # ./mkfs.btrfs -f testfile : SMALL VOLUME: forcing mixed metadata/data groups no space to alloc data/metadata chunk failed to setup the root directory TBD is calculate minimum size for setting and put it in the error message to let user know how large amount of volume is required. Signed-off-by: Hidetoshi Seto seto.hideto...@jp.fujitsu.com --- mkfs.c | 17 + 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/mkfs.c b/mkfs.c index a98fe54..bac122f 100644 --- a/mkfs.c +++ b/mkfs.c @@ -81,6 +81,11 @@ static int make_root_dir(struct btrfs_root *root, int mixed) chunk_start, chunk_size, BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA); + if (ret == -ENOSPC) { + fprintf(stderr, + no space to alloc data/metadata chunk\n); + goto err; + } BUG_ON(ret); ret = btrfs_make_block_group(trans, root, 0, BTRFS_BLOCK_GROUP_METADATA | @@ -93,6 +98,10 @@ static int make_root_dir(struct btrfs_root *root, int mixed) ret = btrfs_alloc_chunk(trans, root-fs_info-extent_root, chunk_start, chunk_size, BTRFS_BLOCK_GROUP_METADATA); + if (ret == -ENOSPC) { + fprintf(stderr, no space to alloc metadata chunk\n); + goto err; + } BUG_ON(ret); ret = btrfs_make_block_group(trans, root, 0, BTRFS_BLOCK_GROUP_METADATA, @@ -110,6 +119,10 @@ static int make_root_dir(struct btrfs_root *root, int mixed) ret = btrfs_alloc_chunk(trans, root-fs_info-extent_root, chunk_start, chunk_size, BTRFS_BLOCK_GROUP_DATA); + if (ret == -ENOSPC) { + fprintf(stderr, no space to alloc data chunk\n); + goto err; + } BUG_ON(ret); ret = btrfs_make_block_group(trans, root, 0, BTRFS_BLOCK_GROUP_DATA, @@ -181,6 +194,10 @@ static int create_one_raid_group(struct btrfs_trans_handle *trans, ret = btrfs_alloc_chunk(trans, root-fs_info-extent_root, chunk_start, chunk_size, type); + if (ret == -ENOSPC) { + fprintf(stderr, not enough free space\n); + exit(1); + } BUG_ON(ret); ret = btrfs_make_block_group(trans, root-fs_info-extent_root, 0, type, BTRFS_FIRST_CHUNK_TREE_OBJECTID, -- 1.7.1 -- 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 3/3] btrfs-progs: calculate available blocks on device properly
I found that mkfs.btrfs aborts when assigned multi volumes contain a small volume: # parted /dev/sdf p Model: LSI MegaRAID SAS RMB (scsi) Disk /dev/sdf: 72.8GB Sector size (logical/physical): 512B/512B Partition Table: msdos Number Start End SizeType File system Flags 1 32.3kB 72.4GB 72.4GB primary 2 72.4GB 72.8GB 461MB primary # ./mkfs.btrfs -f /dev/sdf1 /dev/sdf2 : SMALL VOLUME: forcing mixed metadata/data groups adding device /dev/sdf2 id 2 mkfs.btrfs: volumes.c:852: btrfs_alloc_chunk: Assertion `!(ret)' failed. Aborted (core dumped) This failure of btrfs_alloc_chunk was caused by following steps: 1) since there is only small space in the small device, mkfs was going to allocate a chunk from free space as much as available. So mkfs called btrfs_alloc_chunk with size = device-total_bytes - device-used_bytes. 2) (According to the comment in source code, to avoid overwriting superblock,) btrfs_alloc_chunk starts taking chunks at an offset of 1MB. It means that the layout of a disk will be like: [[1MB at beginning for sb][allocated chunks]* ... free space ... ] and you can see that the available free space for allocation is: avail = device-total_bytes - device-used_bytes - 1MB. 3) Therefore there is only free space 1MB less than requested. damn. From further investigations I also found that this issue is easily reproduced by using -A, --alloc-start option: # truncate --size=1G testfile # ./mkfs.btrfs -A900M -f testfile : mkfs.btrfs: volumes.c:852: btrfs_alloc_chunk: Assertion `!(ret)' failed. Aborted (core dumped) In this case there is only 100MB for allocation but btrfs_alloc_chunk was going to allocate more than the 100MB. The root cause of both of above troubles is a same simple bug: btrfs_chunk_alloc does not calculate available bytes properly even though it researches how many devices have enough room to have a chunk to be allocated. So this patch introduces new function btrfs_device_avail_bytes() which returns available bytes for allocation in specified device. Signed-off-by: Hidetoshi Seto seto.hideto...@jp.fujitsu.com --- ctree.h |8 + volumes.c | 104 +--- 2 files changed, 106 insertions(+), 6 deletions(-) diff --git a/ctree.h b/ctree.h index 0b0d701..90be7ab 100644 --- a/ctree.h +++ b/ctree.h @@ -811,6 +811,14 @@ struct btrfs_csum_item { u8 csum; } __attribute__ ((__packed__)); +/* + * We don't want to overwrite 1M at the beginning of device, even though + * there is our 1st superblock at 64k. Some possible reasons: + * - the first 64k blank is useful for some boot loader/manager + * - the first 1M could be scratched by buggy partitioner or somesuch + */ +#define BTRFS_BLOCK_RESERVED_1M_FOR_SUPER ((u64)1024 * 1024) + /* tag for the radix tree of block groups in ram */ #define BTRFS_BLOCK_GROUP_DATA (1ULL 0) #define BTRFS_BLOCK_GROUP_SYSTEM (1ULL 1) diff --git a/volumes.c b/volumes.c index 0ff2283..e8d7f25 100644 --- a/volumes.c +++ b/volumes.c @@ -268,7 +268,7 @@ static int find_free_dev_extent(struct btrfs_trans_handle *trans, struct btrfs_dev_extent *dev_extent = NULL; u64 hole_size = 0; u64 last_byte = 0; - u64 search_start = 0; + u64 search_start = root-fs_info-alloc_start; u64 search_end = device-total_bytes; int ret; int slot = 0; @@ -283,10 +283,12 @@ static int find_free_dev_extent(struct btrfs_trans_handle *trans, /* we don't want to overwrite the superblock on the drive, * so we make sure to start at an offset of at least 1MB */ - search_start = max((u64)1024 * 1024, search_start); + search_start = max(BTRFS_BLOCK_RESERVED_1M_FOR_SUPER, search_start); - if (root-fs_info-alloc_start + num_bytes = device-total_bytes) - search_start = max(root-fs_info-alloc_start, search_start); + if (search_start = search_end) { + ret = -ENOSPC; + goto error; + } key.objectid = device-devid; key.offset = search_start; @@ -660,6 +662,94 @@ static u32 find_raid56_stripe_len(u32 data_devices, u32 dev_stripe_target) return 64 * 1024; } +/* + * btrfs_device_avail_bytes - count bytes available for alloc_chunk + * + * It is not equal to device-total_bytes - device-bytes_used. + * We do not allocate any chunk in 1M at beginning of device, and not + * allowed to allocate any chunk before alloc_start if it is specified. + * So search holes from max(1M, alloc_start) to device-total_bytes. + */ +static int btrfs_device_avail_bytes(struct btrfs_trans_handle *trans, + struct btrfs_device *device, + u64 *avail_bytes) +{ + struct btrfs_path *path; + struct btrfs_root *root = device-dev_root; + struct btrfs_key key; +
Re: [PATCH 5/5] btrfs-progs:free strdup()s that are not freed
On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote: The strdup()s not freed are reported as memory leaks by valgrind. Signed-off-by: Gui Hecheng guihc.f...@cn.fujitsu.com --- cmds-subvolume.c | 48 ++-- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e1fa81a..51c529c 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv) { int retval, res, len; int fddst = -1; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; char*dst; @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv) goto out; } - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); if (!strcmp(newname, .) || !strcmp(newname, ..) || strchr(newname, '/') ){ @@ -175,6 +177,11 @@ out: close_file_or_dir(fddst, dirstream); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + free(3) already checks the pointer for NULL, no need to do it on your own. return retval; } @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv) int res, fd, len, e, cnt = 1, ret = 0; struct btrfs_ioctl_vol_args args; char*dname, *vname, *cpath; + char*dupdname = NULL; + char*dupvname = NULL; char*path; DIR *dirstream = NULL; @@ -230,10 +239,10 @@ again: } cpath = realpath(path, NULL); - dname = strdup(cpath); - dname = dirname(dname); - vname = strdup(cpath); - vname = basename(vname); + dupdname = strdup(cpath); + dname = dirname(dupdname); + dupvname = strdup(cpath); + vname = basename(dupvname); free(cpath); if( !strcmp(vname,.) || !strcmp(vname,..) || @@ -274,6 +283,10 @@ again: } out: + if (dupdname != NULL) + free(dupdname); + if (dupvname != NULL) + free(dupvname); Here again. cnt++; if (cnt argc) goto again; @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv) int res, retval; int fd = -1, fddst = -1; int len, readonly = 0; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; struct btrfs_ioctl_vol_args_v2 args; @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv) } if (res 0) { - newname = strdup(subvol); - newname = basename(newname); + dupname = strdup(subvol); + newname = basename(dupname); dstdir = dst; } else { - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); } if (!strcmp(newname, .) || !strcmp(newname, ..) || @@ -630,6 +645,11 @@ out: close_file_or_dir(fd, dirstream2); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + And here. return retval; } -- 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 5/5] btrfs-progs:free strdup()s that are not freed
On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote: On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote: The strdup()s not freed are reported as memory leaks by valgrind. Signed-off-by: Gui Hecheng guihc.f...@cn.fujitsu.com --- cmds-subvolume.c | 48 ++-- 1 file changed, 34 insertions(+), 14 deletions(-) Just noticed that you had already sent a V2 with the things fixed that I have commented, but you sent the 5/5 as a 1/5 and added the changelog v1-v2 none which made it difficult to recognize :). But maybe David Sterba is smart enough when he picks up the patches. diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e1fa81a..51c529c 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c [...] @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv) { int retval, res, len; int fddst = -1; +char*dupname = NULL; +char*dupdir = NULL; char*newname; char*dstdir; char*dst; @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv) goto out; } -newname = strdup(dst); -newname = basename(newname); -dstdir = strdup(dst); -dstdir = dirname(dstdir); +dupname = strdup(dst); +newname = basename(dupname); +dupdir = strdup(dst); +dstdir = dirname(dupdir); if (!strcmp(newname, .) || !strcmp(newname, ..) || strchr(newname, '/') ){ @@ -175,6 +177,11 @@ out: close_file_or_dir(fddst, dirstream); free(inherit); +if (dupname != NULL) +free(dupname); +if (dupdir != NULL) +free(dupdir); + free(3) already checks the pointer for NULL, no need to do it on your own. return retval; } @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv) int res, fd, len, e, cnt = 1, ret = 0; struct btrfs_ioctl_vol_args args; char*dname, *vname, *cpath; +char*dupdname = NULL; +char*dupvname = NULL; char*path; DIR *dirstream = NULL; @@ -230,10 +239,10 @@ again: } cpath = realpath(path, NULL); -dname = strdup(cpath); -dname = dirname(dname); -vname = strdup(cpath); -vname = basename(vname); +dupdname = strdup(cpath); +dname = dirname(dupdname); +dupvname = strdup(cpath); +vname = basename(dupvname); free(cpath); if( !strcmp(vname,.) || !strcmp(vname,..) || @@ -274,6 +283,10 @@ again: } out: +if (dupdname != NULL) +free(dupdname); +if (dupvname != NULL) +free(dupvname); Here again. cnt++; if (cnt argc) goto again; @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv) int res, retval; int fd = -1, fddst = -1; int len, readonly = 0; +char*dupname = NULL; +char*dupdir = NULL; char*newname; char*dstdir; struct btrfs_ioctl_vol_args_v2 args; @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv) } if (res 0) { -newname = strdup(subvol); -newname = basename(newname); +dupname = strdup(subvol); +newname = basename(dupname); dstdir = dst; } else { -newname = strdup(dst); -newname = basename(newname); -dstdir = strdup(dst); -dstdir = dirname(dstdir); +dupname = strdup(dst); +newname = basename(dupname); +dupdir = strdup(dst); +dstdir = dirname(dupdir); } if (!strcmp(newname, .) || !strcmp(newname, ..) || @@ -630,6 +645,11 @@ out: close_file_or_dir(fd, dirstream2); free(inherit); +if (dupname != NULL) +free(dupname); +if (dupdir != NULL) +free(dupdir); + And here. return retval; } -- 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 18/20] Btrfs-progs: fix magic return value in cmds-balance.c
Hi Illya, On 09/05/2013 12:26 AM, Ilya Dryomov wrote: Hi Wang, Thank you for doing the grunt work, it's been a long standing todo. See the comments below. On Wed, Sep 4, 2013 at 6:22 PM, Wang Shilong wangshilong1...@gmail.com wrote: From: Wang Shilong wangsl.f...@cn.fujitsu.com If there is no balance in progress.resume/pause/cancel will return 2. For usage or syntal errors will return 1. 0 means operations return successfully. This needs to be reworded (spelling, punctuation). will fix this. Signed-off-by: Wang Shilong wangsl.f...@cn.fujitsu.com --- cmds-balance.c | 93 -- 1 file changed, 58 insertions(+), 35 deletions(-) diff --git a/cmds-balance.c b/cmds-balance.c index b7382ef..fd68051 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -58,7 +58,7 @@ static int parse_one_profile(const char *profile, u64 *flags) *flags |= BTRFS_AVAIL_ALLOC_BIT_SINGLE; } else { fprintf(stderr, Unknown profile '%s'\n, profile); - return 1; + return -ENOENT; } return 0; @@ -68,12 +68,14 @@ static int parse_profiles(char *profiles, u64 *flags) { char *this_char; char *save_ptr = NULL; /* Satisfy static checkers */ + int ret; for (this_char = strtok_r(profiles, |, save_ptr); this_char != NULL; this_char = strtok_r(NULL, |, save_ptr)) { - if (parse_one_profile(this_char, flags)) - return 1; + ret = parse_one_profile(this_char, flags); + if (ret) + return ret; } return 0; @@ -86,7 +88,7 @@ static int parse_u64(const char *str, u64 *result) val = strtoull(str, endptr, 10); if (*endptr) - return 1; + return -EINVAL; *result = val; return 0; @@ -95,6 +97,7 @@ static int parse_u64(const char *str, u64 *result) static int parse_range(const char *range, u64 *start, u64 *end) { char *dots; + int ret; dots = strstr(range, ..); if (dots) { @@ -107,29 +110,31 @@ static int parse_range(const char *range, u64 *start, u64 *end) *end = (u64)-1; skipped++; } else { - if (parse_u64(rest, end)) - return 1; + ret = parse_u64(rest, end); + if (ret) + return ret; } if (dots == range) { *start = 0; skipped++; } else { + ret = parse_u64(rest, end); if (parse_u64(range, start)) - return 1; + return ret; } if (*start = *end) { fprintf(stderr, Range %llu..%llu doesn't make sense\n, (unsigned long long)*start, (unsigned long long)*end); - return 1; + return -EINVAL; } if (skipped = 1) return 0; } - return 1; + return -EINVAL; } static int parse_filters(char *filters, struct btrfs_balance_args *args) @@ -137,6 +142,7 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) char *this_char; char *value; char *save_ptr = NULL; /* Satisfy static checkers */ + int ret = 0; if (!filters) return 0; @@ -150,70 +156,72 @@ static int parse_filters(char *filters, struct btrfs_balance_args *args) if (!value || !*value) { fprintf(stderr, the profiles filter requires an argument\n); - return 1; + return -EINVAL; } if (parse_profiles(value, args-profiles)) { fprintf(stderr, Invalid profiles argument\n); - return 1; + return -EINVAL; } args-flags |= BTRFS_BALANCE_ARGS_PROFILES; } else if (!strcmp(this_char, usage)) { if (!value || !*value) { fprintf(stderr, the usage filter requires an argument\n); - return 1; + return -EINVAL; } if (parse_u64(value, args-usage) || args-usage 100) {
[PATCH V3 5/5] btrfs-progs:free strdup()s that are not freed
The strdup()s not freed are reported as memory leaks by valgrind. Signed-off-by: Gui Hecheng guihc.f...@cn.fujitsu.com --- Changelog V1 - V2: - remove the if decision before free() Changelog V2 - V3: - correct confusing subject and add changelog --- cmds-subvolume.c | 40 ++-- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e1fa81a..f7a0c5f 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv) { int retval, res, len; int fddst = -1; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; char*dst; @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv) goto out; } - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); if (!strcmp(newname, .) || !strcmp(newname, ..) || strchr(newname, '/') ){ @@ -174,6 +176,8 @@ static int cmd_subvol_create(int argc, char **argv) out: close_file_or_dir(fddst, dirstream); free(inherit); + free(dupname); + free(dupdir); return retval; } @@ -208,6 +212,8 @@ static int cmd_subvol_delete(int argc, char **argv) int res, fd, len, e, cnt = 1, ret = 0; struct btrfs_ioctl_vol_args args; char*dname, *vname, *cpath; + char*dupdname = NULL; + char*dupvname = NULL; char*path; DIR *dirstream = NULL; @@ -230,10 +236,10 @@ again: } cpath = realpath(path, NULL); - dname = strdup(cpath); - dname = dirname(dname); - vname = strdup(cpath); - vname = basename(vname); + dupdname = strdup(cpath); + dname = dirname(dupdname); + dupvname = strdup(cpath); + vname = basename(dupvname); free(cpath); if( !strcmp(vname,.) || !strcmp(vname,..) || @@ -274,6 +280,8 @@ again: } out: + free(dupdname); + free(dupvname); cnt++; if (cnt argc) goto again; @@ -495,6 +503,8 @@ static int cmd_snapshot(int argc, char **argv) int res, retval; int fd = -1, fddst = -1; int len, readonly = 0; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; struct btrfs_ioctl_vol_args_v2 args; @@ -562,14 +572,14 @@ static int cmd_snapshot(int argc, char **argv) } if (res 0) { - newname = strdup(subvol); - newname = basename(newname); + dupname = strdup(subvol); + newname = basename(dupname); dstdir = dst; } else { - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); } if (!strcmp(newname, .) || !strcmp(newname, ..) || @@ -629,6 +639,8 @@ out: close_file_or_dir(fddst, dirstream1); close_file_or_dir(fd, dirstream2); free(inherit); + free(dupname); + free(dupdir); return retval; } -- 1.8.0.1 -- 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 5/5] btrfs-progs:free strdup()s that are not freed
On Thu, 2013-09-05 at 09:10 +0200, Stefan Behrens wrote: On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote: On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote: The strdup()s not freed are reported as memory leaks by valgrind. Signed-off-by: Gui Hecheng guihc.f...@cn.fujitsu.com --- cmds-subvolume.c | 48 ++-- 1 file changed, 34 insertions(+), 14 deletions(-) Just noticed that you had already sent a V2 with the things fixed that I have commented, but you sent the 5/5 as a 1/5 and added the changelog v1-v2 none which made it difficult to recognize :). But maybe David Sterba is smart enough when he picks up the patches. Thank you for your friendly advice. I will handle it snow. diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e1fa81a..51c529c 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c [...] @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv) { int retval, res, len; int fddst = -1; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; char*dst; @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv) goto out; } - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); if (!strcmp(newname, .) || !strcmp(newname, ..) || strchr(newname, '/') ){ @@ -175,6 +177,11 @@ out: close_file_or_dir(fddst, dirstream); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + free(3) already checks the pointer for NULL, no need to do it on your own. return retval; } @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv) int res, fd, len, e, cnt = 1, ret = 0; struct btrfs_ioctl_vol_args args; char*dname, *vname, *cpath; + char*dupdname = NULL; + char*dupvname = NULL; char*path; DIR *dirstream = NULL; @@ -230,10 +239,10 @@ again: } cpath = realpath(path, NULL); - dname = strdup(cpath); - dname = dirname(dname); - vname = strdup(cpath); - vname = basename(vname); + dupdname = strdup(cpath); + dname = dirname(dupdname); + dupvname = strdup(cpath); + vname = basename(dupvname); free(cpath); if( !strcmp(vname,.) || !strcmp(vname,..) || @@ -274,6 +283,10 @@ again: } out: + if (dupdname != NULL) + free(dupdname); + if (dupvname != NULL) + free(dupvname); Here again. cnt++; if (cnt argc) goto again; @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv) int res, retval; int fd = -1, fddst = -1; int len, readonly = 0; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; struct btrfs_ioctl_vol_args_v2 args; @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv) } if (res 0) { - newname = strdup(subvol); - newname = basename(newname); + dupname = strdup(subvol); + newname = basename(dupname); dstdir = dst; } else { - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); } if (!strcmp(newname, .) || !strcmp(newname, ..) || @@ -630,6 +645,11 @@ out: close_file_or_dir(fd, dirstream2); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + And here. return retval; } -- 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 5/5] btrfs-progs:free strdup()s that are not freed
On 09/05/2013 04:08 PM, Gui Hecheng wrote: On Thu, 2013-09-05 at 09:10 +0200, Stefan Behrens wrote: On Thu, 05 Sep 2013 09:00:07 +0200, Stefan Behrens wrote: On Thu, 5 Sep 2013 10:38:58 +0800, Gui Hecheng wrote: The strdup()s not freed are reported as memory leaks by valgrind. Signed-off-by: Gui Hecheng guihc.f...@cn.fujitsu.com --- cmds-subvolume.c | 48 ++-- 1 file changed, 34 insertions(+), 14 deletions(-) Just noticed that you had already sent a V2 with the things fixed that I have commented, but you sent the 5/5 as a 1/5 and added the changelog v1-v2 none which made it difficult to recognize :). But maybe David Sterba is smart enough when he picks up the patches. Thank you for your friendly advice. I will handle it snow. s/snow/soon ^_^ Thanks, Wang diff --git a/cmds-subvolume.c b/cmds-subvolume.c index e1fa81a..51c529c 100644 --- a/cmds-subvolume.c +++ b/cmds-subvolume.c [...] @@ -75,6 +75,8 @@ static int cmd_subvol_create(int argc, char **argv) { int retval, res, len; int fddst = -1; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; char*dst; @@ -119,10 +121,10 @@ static int cmd_subvol_create(int argc, char **argv) goto out; } - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); if (!strcmp(newname, .) || !strcmp(newname, ..) || strchr(newname, '/') ){ @@ -175,6 +177,11 @@ out: close_file_or_dir(fddst, dirstream); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + free(3) already checks the pointer for NULL, no need to do it on your own. return retval; } @@ -208,6 +215,8 @@ static int cmd_subvol_delete(int argc, char **argv) int res, fd, len, e, cnt = 1, ret = 0; struct btrfs_ioctl_vol_args args; char*dname, *vname, *cpath; + char*dupdname = NULL; + char*dupvname = NULL; char*path; DIR *dirstream = NULL; @@ -230,10 +239,10 @@ again: } cpath = realpath(path, NULL); - dname = strdup(cpath); - dname = dirname(dname); - vname = strdup(cpath); - vname = basename(vname); + dupdname = strdup(cpath); + dname = dirname(dupdname); + dupvname = strdup(cpath); + vname = basename(dupvname); free(cpath); if( !strcmp(vname,.) || !strcmp(vname,..) || @@ -274,6 +283,10 @@ again: } out: + if (dupdname != NULL) + free(dupdname); + if (dupvname != NULL) + free(dupvname); Here again. cnt++; if (cnt argc) goto again; @@ -495,6 +508,8 @@ static int cmd_snapshot(int argc, char **argv) int res, retval; int fd = -1, fddst = -1; int len, readonly = 0; + char*dupname = NULL; + char*dupdir = NULL; char*newname; char*dstdir; struct btrfs_ioctl_vol_args_v2 args; @@ -562,14 +577,14 @@ static int cmd_snapshot(int argc, char **argv) } if (res 0) { - newname = strdup(subvol); - newname = basename(newname); + dupname = strdup(subvol); + newname = basename(dupname); dstdir = dst; } else { - newname = strdup(dst); - newname = basename(newname); - dstdir = strdup(dst); - dstdir = dirname(dstdir); + dupname = strdup(dst); + newname = basename(dupname); + dupdir = strdup(dst); + dstdir = dirname(dupdir); } if (!strcmp(newname, .) || !strcmp(newname, ..) || @@ -630,6 +645,11 @@ out: close_file_or_dir(fd, dirstream2); free(inherit); + if (dupname != NULL) + free(dupname); + if (dupdir != NULL) + free(dupdir); + And here. return retval; } -- 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 -- 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 18/20] Btrfs-progs: fix magic return value in cmds-balance.c
On Thu, 05 Sep 2013 15:44:11 +0800, Wang Shilong wrote: [..] @@ -297,9 +305,10 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args, DIR *dirstream = NULL; fd = open_file_or_dir(path, dirstream); + e = errno; if (fd 0) { fprintf(stderr, ERROR: can't access to '%s'\n, path); - return 12; + return e; Since I didn't understand whether you rejected or acknowledged Ilya's comments, if you don't do so, please change the above line to return -e like it is everywhere else: errno is returned as a negative value, 0 means no error, a positive value means the function failed but the return value cannot be interpreted as an errno. -- 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 18/20] Btrfs-progs: fix magic return value in cmds-balance.c
On 09/05/2013 04:21 PM, Stefan Behrens wrote: On Thu, 05 Sep 2013 15:44:11 +0800, Wang Shilong wrote: [..] @@ -297,9 +305,10 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args, DIR *dirstream = NULL; fd = open_file_or_dir(path, dirstream); + e = errno; if (fd 0) { fprintf(stderr, ERROR: can't access to '%s'\n, path); - return 12; + return e; Since I didn't understand whether you rejected or acknowledged Ilya's My answer: acknowledged. Will send a V2 later, just wait more comments. Thanks, Wang comments, if you don't do so, please change the above line to return -e like it is everywhere else: errno is returned as a negative value, 0 means no error, a positive value means the function failed but the return value cannot be interpreted as an errno. -- 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 -- 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 00/20] fix magic return value in btrfs-progs
On 09/05/2013 10:14 AM, Anand Jain wrote: On 09/04/2013 11:22 PM, Wang Shilong wrote: This patchset tries to fix all the magic return value in btrfs-progs. Most commands will have three kinds of return value: 0 success 1 usage of syntax errors Exceptions come from balance/scrub/replace. For example, replace cancel will return 2 if there is no operations in progress. Thanks for writing this much needed. Its better to have these return error codes defined in a header. So that it would guide the future developments. Agree. We also need update man page.^_^ Thanks, Wang Anand -- 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 -- 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 3/3] Btrfs: fix several potential problems in copy_nocow_pages_for_inode
On Thu, 27 Jun 2013 18:51:00 +0800, Miao Xie wrote: - It makes no sense that we deal with a inode in the dead tree. This caused that the replace procedure was not dealing with free space cache entries anymore (which have btrfs_root_refs() == 0). I accidentally fixed it as a side-effect of Btrf: cleanup: don't check for root_refs == 0 twice and intentionally fixed it with Btrfs: eliminate the exceptional root_tree refs=0 (which is not yet sent). - fix the race between dio and page copy by waiting the dio completion This causes lockdep issues which I've seen once after running './check -g all' followed by './check btrfs/005' during the 005 run, and on a second box once while running the btrfs xfstests 001 up to 011 during the xfstest 011 run, both boxes were running the latest btrfs-next plus the patch Btrfs: eliminate the exceptional root_tree refs=0 (but I do not think that the latter patch matters). I'm unable to reproduce this lockdep warning, but it seems to make sense, see below. ### scrub.c: static void copy_nocow_pages_worker(struct btrfs_work *work) { ... trans = btrfs_join_transaction(root); ... ret = iterate_inodes_from_logical(logical, fs_info, path, copy_nocow_pages_for_inode, nocow_ctx); ... btrfs_end_transaction(trans, root); ... } static int copy_nocow_pages_for_inode(u64 inum, u64 offset, u64 root, void *ctx) { ... /* Avoid truncate/dio/punch hole.. */ mutex_lock(inode-i_mutex); inode_dio_wait(inode); ... mutex_unlock(inode-i_mutex); ... } ### file.c: int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync) { ... mutex_lock(inode-i_mutex); ... trans = btrfs_start_transaction(root, 0); ... mutex_unlock(inode-i_mutex); ... ret = btrfs_end_transaction(trans, root); ... } [ INFO: possible circular locking dependency detected ] 3.10.0+ #5 Not tainted --- xfs_io/30404 is trying to acquire lock: (sb_internal){.+.+..}, at: [a00abad6] start_transaction+0x296/0x4f0 [btrfs] but task is already holding lock: (sb-s_type-i_mutex_key#11){+.+.+.}, at: [a00bd029] btrfs_sync_file+0xb9/0x2c0 [btrfs] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: - #1 (sb-s_type-i_mutex_key#11){+.+.+.}: [810e564a] lock_acquire+0x8a/0x120 [81993f13] mutex_lock_nested+0x73/0x390 [a01005d5] copy_nocow_pages_for_inode+0x105/0x2b0 [btrfs] [a010941e] iterate_extent_inodes+0x1ce/0x300 [btrfs] [a01095f7] iterate_inodes_from_logical+0xa7/0xb0 [btrfs] [a00fffeb] copy_nocow_pages_worker+0x9b/0x160 [btrfs] [a00d9b1f] worker_loop+0x13f/0x5b0 [btrfs] [810abd36] kthread+0xd6/0xe0 [8199f66c] ret_from_fork+0x7c/0xb0 - #0 (sb_internal){.+.+..}: [810e4f42] __lock_acquire+0x1d12/0x1e10 [810e564a] lock_acquire+0x8a/0x120 [8119d7ab] __sb_start_write+0xdb/0x1c0 [a00abad6] start_transaction+0x296/0x4f0 [btrfs] [a00ac0e6] btrfs_start_transaction+0x16/0x20 [btrfs] [a00bd0a9] btrfs_sync_file+0x139/0x2c0 [btrfs] [811c9848] do_fsync+0x58/0x80 [811c9adb] SyS_fsync+0xb/0x10 [8199f712] system_call_fastpath+0x16/0x1b other info that might help us debug this: Possible unsafe locking scenario: CPU0CPU1 lock(sb-s_type-i_mutex_key#11); lock(sb_internal); lock(sb-s_type-i_mutex_key#11); lock(sb_internal); *** DEADLOCK *** 1 lock held by xfs_io/30404: #0: (sb-s_type-i_mutex_key#11){+.+.+.}, at: [a00bd029] btrfs_sync_file+0xb9/0x2c0 [btrfs] stack backtrace: CPU: 3 PID: 30404 Comm: xfs_io Not tainted 3.10.0+ #5 Hardware name: Supermicro X9SCL/X9SCM/X9SCL/X9SCM, BIOS 2.0b 09/17/2012 865d9790 880804ef5bb8 8198eaba 880804ef5c08 81989a92 03d0 880804ef5c98 880804d786d0 880804d78708 880804d786d0 880804d78000 Call Trace: [8198eaba] dump_stack+0x19/0x1b [81989a92] print_circular_bug+0x1fe/0x20f [810e4f42] __lock_acquire+0x1d12/0x1e10 [810e564a] lock_acquire+0x8a/0x120 [a00abad6] ? start_transaction+0x296/0x4f0 [btrfs] [8119d7ab] __sb_start_write+0xdb/0x1c0 [a00abad6] ? start_transaction+0x296/0x4f0 [btrfs] [a00c2db1] ? btrfs_put_ordered_extent+0x71/0xd0 [btrfs] [a00abad6] ? start_transaction+0x296/0x4f0 [btrfs] [a00ab939] ? start_transaction+0xf9/0x4f0 [btrfs] [811920ef] ? kmem_cache_alloc+0xdf/0x1b0 [a00ab939] ? start_transaction+0xf9/0x4f0 [btrfs]
btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. With best regards, Timofey. -- 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: eliminate the exceptional root_tree refs=0
The fact that btrfs_root_refs() returned 0 for the tree_root caused bugs in the past, therefore it is set to 1 with this patch and (hopefully) all affected code is adapted to this change. I verified this change by temporarily adding WARN_ON() checks everywhere where btrfs_root_refs() is used, checking whether the logic of the code is changed by btrfs_root_refs() returning 1 instead of 0 for root-root_key.objectid == BTRFS_ROOT_TREE_OBJECTID. With these added checks, I ran the xfstests './check -g auto'. The two roots chunk_root and log_root_tree that are only referenced by the superblock and the log_roots below the log_root_tree still have btrfs_root_refs() == 0, only the tree_root is changed. Signed-off-by: Stefan Behrens sbehr...@giantdisaster.de --- fs/btrfs/disk-io.c | 1 + fs/btrfs/inode-map.c | 3 +-- fs/btrfs/inode.c | 21 - 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index fa8b2c6..ffc3e43 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2667,6 +2667,7 @@ retry_root_backup: btrfs_set_root_node(tree_root-root_item, tree_root-node); tree_root-commit_root = btrfs_root_node(tree_root); + btrfs_set_root_refs(tree_root-root_item, 1); location.objectid = BTRFS_EXTENT_TREE_OBJECTID; location.type = BTRFS_ROOT_ITEM_KEY; diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c index 2c66ddb..d11e1c6 100644 --- a/fs/btrfs/inode-map.c +++ b/fs/btrfs/inode-map.c @@ -412,8 +412,7 @@ int btrfs_save_ino_cache(struct btrfs_root *root, return 0; /* Don't save inode cache if we are deleting this root */ - if (btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) + if (btrfs_root_refs(root-root_item) == 0) return 0; if (!btrfs_test_opt(root, INODE_MAP_CACHE)) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 26992ee..7d0ef55 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4472,8 +4472,10 @@ void btrfs_evict_inode(struct inode *inode) trace_btrfs_inode_evict(inode); truncate_inode_pages(inode-i_data, 0); - if (inode-i_nlink (btrfs_root_refs(root-root_item) != 0 || - btrfs_is_free_space_inode(inode))) + if (inode-i_nlink + ((btrfs_root_refs(root-root_item) != 0 + root-root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) || +btrfs_is_free_space_inode(inode))) goto no_delete; if (is_bad_inode(inode)) { @@ -4490,7 +4492,8 @@ void btrfs_evict_inode(struct inode *inode) } if (inode-i_nlink 0) { - BUG_ON(btrfs_root_refs(root-root_item) != 0); + BUG_ON(btrfs_root_refs(root-root_item) != 0 + root-root_key.objectid != BTRFS_ROOT_TREE_OBJECTID); goto no_delete; } @@ -4731,14 +4734,7 @@ static void inode_tree_del(struct inode *inode) } spin_unlock(root-inode_lock); - /* -* Free space cache has inodes in the tree root, but the tree root has a -* root_refs of 0, so this could end up dropping the tree root as a -* snapshot, so we need the extra !root-fs_info-tree_root check to -* make sure we don't drop it. -*/ - if (empty btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) { + if (empty btrfs_root_refs(root-root_item) == 0) { synchronize_srcu(root-fs_info-subvol_srcu); spin_lock(root-inode_lock); empty = RB_EMPTY_ROOT(root-inode_tree); @@ -7872,8 +7868,7 @@ int btrfs_drop_inode(struct inode *inode) return 1; /* the snap/subvol tree is on deleting */ - if (btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) + if (btrfs_root_refs(root-root_item) == 0) return 1; else return generic_drop_inode(inode); -- 1.8.4 -- 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: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On Thu, 5 Sep 2013 15:54:07 +0100 Hugo Mills h...@carfax.org.uk wrote: On Thu, Sep 05, 2013 at 05:43:27PM +0300, Тимофей Титовец wrote: Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. I'm guessing here, but I suspect you will need to create a loopback device so that btrfs-convert can look at it as a block device rather than as a file: # losetup -f --show file /dev/loop0 # btrfs-convert /dev/loop0 Hugo. Nope, just today I saw someone report the same problem in a blog comment: http://popey.com/blog/2013/09/02/fun-with-btrfs-on-ubuntu/#comment-9704 --- # umount /dev/sdb1 # fsck -f /dev/sdb1 fsck из util-linux 2.20.1 e2fsck 1.42.8 (20-Jun-2013) data500: 144653/30531584 files (0.9% non-contiguous), 102659367/122096384 blocks # btrfs-convert /dev/sdb1 No valid Btrfs found on /dev/sdb1 unable to open ctree conversion aborted. Ubuntu 13.10 btrfs-tools 0.19+20130705-1 --- It looks like a bug in btrfs-convert. -- With respect, Roman signature.asc Description: PGP signature
Re: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On Thu, Sep 05, 2013 at 09:06:19PM +0600, Roman Mamedov wrote: On Thu, 5 Sep 2013 15:54:07 +0100 Hugo Mills h...@carfax.org.uk wrote: On Thu, Sep 05, 2013 at 05:43:27PM +0300, Тимофей Титовец wrote: Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. I'm guessing here, but I suspect you will need to create a loopback device so that btrfs-convert can look at it as a block device rather than as a file: # losetup -f --show file /dev/loop0 # btrfs-convert /dev/loop0 Hugo. Nope, just today I saw someone report the same problem in a blog comment: http://popey.com/blog/2013/09/02/fun-with-btrfs-on-ubuntu/#comment-9704 It's the same person, in fact. I'd not seen that the one on popey's blog was doing it with block devices. This does indeed look like a fairly drastic bug... Hugo. -- === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk === PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk --- Is it true that last known good on Windows XP --- boots into CP/M? signature.asc Description: Digital signature
Re: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On Thu, 5 Sep 2013 16:30:23 +0100 Hugo Mills h...@carfax.org.uk wrote: Nope, just today I saw someone report the same problem in a blog comment: http://popey.com/blog/2013/09/02/fun-with-btrfs-on-ubuntu/#comment-9704 It's the same person, in fact. FWIW both names are Cyrillic but they are different. -- With respect, Roman signature.asc Description: PGP signature
Re: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On Thu, Sep 05, 2013 at 05:43:27PM +0300, Тимофей Титовец wrote: Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. I'm guessing here, but I suspect you will need to create a loopback device so that btrfs-convert can look at it as a block device rather than as a file: # losetup -f --show file /dev/loop0 # btrfs-convert /dev/loop0 Hugo. -- === Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk === PGP key: 65E74AC0 from wwwkeys.eu.pgp.net or http://www.carfax.org.uk --- Eighth Army Push Bottles Up Germans -- WWII newspaper --- headline (possibly apocryphal) signature.asc Description: Digital signature
Re: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On 9/5/13 9:43 AM, Тимофей Титовец wrote: Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. This was a regression around July 3; there was no regression test at the time. [615f2867854c186a37cb2e2e5a2e13e9ed4ab0df] Btrfs-progs: cleanup similar code in open_ctree_* and close_ctree broke it. Patches were sent to the list to fix it on July 17, https://patchwork.kernel.org/patch/2828820/ but they haven't been merged into the main repo. I sent a regression test for it to the list on Aug 4, but nobody reviewed it, so it hasn't been merged into the test suite, either. Winning all around! -Eric -- 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: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1
On Thu, Sep 05, 2013 at 10:45:23AM -0500, Eric Sandeen wrote: On 9/5/13 9:43 AM, Тимофей Титовец wrote: Hello guys, i try to convert ext4 volume, but btrfs-convert show me error: No valid Btrfs found on file unable to open ctree conversion aborted. Ubuntu 13.04 Kernel: 3.11 btrfs-progs git version 0.20-git20130822~194aa4a13 way to reproduce error: $ truncate -s 4G file $ mkfs.ext4 file #say yes to create fs on non block device. $ btrfs-convert file No valid Btrfs found on file unable to open ctree conversion aborted. This was a regression around July 3; there was no regression test at the time. [615f2867854c186a37cb2e2e5a2e13e9ed4ab0df] Btrfs-progs: cleanup similar code in open_ctree_* and close_ctree broke it. Patches were sent to the list to fix it on July 17, https://patchwork.kernel.org/patch/2828820/ but they haven't been merged into the main repo. I sent a regression test for it to the list on Aug 4, but nobody reviewed it, so it hasn't been merged into the test suite, either. Winning all around! Alright, alright I'll review it, Jesus. ;), Josef -- 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] xfstests btrfs/309: test btrfs-convert
On Sun, Aug 04, 2013 at 03:12:31PM -0500, Eric Sandeen wrote: Turns out btrfs-convert broke on July 3, and lo! we do not have a regression test, and now we have one, and there was much rejoicing. Signed-off-by: Eric Sandeen sand...@redhat.com --- diff --git a/tests/btrfs/309 b/tests/btrfs/309 new file mode 100755 index 000..acb2d6d --- /dev/null +++ b/tests/btrfs/309 @@ -0,0 +1,118 @@ +#! /bin/bash +# FS QA Test No. 309 +# +# Test btrfs-convert +# +# 1) create ext4 filesystem populate it +# 2) convert it to btrfs, mount it, verify contents +# 3) verify archived ext4 image integriy contents +# 4) populate btrfs fs with new data +# 5) roll back conversion to original ext4 +# 6) verify rolled-back fs integrity contents +# +#--- +# Copyright (c) 2013 Red Hat, Inc. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#--- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo == QA output created by $seq + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap _cleanup; exit \$status 0 1 2 3 15 + +_cleanup() +{ +cd / +rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter.btrfs + +# real QA test starts here + +# Modify as appropriate. +_supported_fs btrfs +_supported_os Linux +_require_scratch + +BTRFS_CONVERT_PROG=`set_prog_path btrfs-convert` +MKFS_EXT4_PROG=`set_prog_path mkfs.ext4` +E2FSCK_PROG=`set_prog_path e2fsck` + +_require_command $BTRFS_CONVERT_PROG btrfs-convert +_require_command $MKFS_EXT4_PROG mkfs.ext4 +_require_command $E2FSCK_PROG e2fsck + +rm -f $seqres.full + +# Create populate an ext4 filesystem +$MKFS_EXT4_PROG -b 4096 $SCRATCH_DEV $seqres.full 21 || \ + _notrun Could not create ext4 filesystem +# Manual mount so we don't use -t btrfs or selinux context +mount -t ext4 $SCRATCH_DEV $SCRATCH_MNT + +cp -aR /lib/modules/`uname -r`/ $SCRATCH_MNT +_scratch_unmount + +# Convert it to btrfs, mount it, verify the data +$BTRFS_CONVERT_PROG $SCRATCH_DEV $seqres.full 21 || \ + _fail btrfs-convert failed +_scratch_mount || _fail Could not mount new btrfs fs +# (Ignore the symlinks which may be broken/nonexistent) +diff -r /lib/modules/`uname -r`/ $SCRATCH_MNT/`uname -r`/ 21 | grep -vw source\|build + +# Old ext4 image file should exist be consistent +$E2FSCK_PROG -fn $SCRATCH_MNT/ext2_saved/image $seqres.full 21 || \ + _fail archived ext4 image is corrupt + +# And the files in that image should match +mkdir -p $SCRATCH_MNT/mnt +mount -o loop $SCRATCH_MNT/ext2_saved/image $SCRATCH_MNT/mnt || \ + _fail could not loop mount saved ext4 image +# Ignore the symlinks which may be broken/nonexistent +diff -r /lib/modules/`uname -r`/ $SCRATCH_MNT/mnt/`uname -r`/ 21 | grep -vw source\|build +umount $SCRATCH_MNT/mnt + +# Now put some fresh data on the btrfs fs +mkdir -p $SCRATCH_MNT/new +cp -aR /lib/modules/`uname -r`/ $SCRATCH_MNT/new + +_scratch_unmount + +# Now restore the ext4 device +$BTRFS_CONVERT_PROG -r $SCRATCH_DEV $seqres.full 21 || \ + _fail btrfs-convert rollback failed + +# Check it again +$E2FSCK_PROG -fn $SCRATCH_DEV $seqres.full 21 || \ +_fail restored ext4 image is corrupt + +# Mount the un-converted ext4 device check the contents +mount -t ext4 $SCRATCH_DEV $SCRATCH_MNT +# (Ignore the symlinks which may be broken/nonexistent) +diff -r /lib/modules/`uname -r`/ $SCRATCH_MNT/`uname -r`/ 21 | grep -vw source\|build + +_scratch_unmount + +# success, all done +status=0 +exit diff --git a/tests/btrfs/309.out b/tests/btrfs/309.out new file mode 100644 index 000..2f5d4a9 --- /dev/null +++ b/tests/btrfs/309.out @@ -0,0 +1 @@ +== QA output created by 309 diff --git a/tests/btrfs/group b/tests/btrfs/group index bc6c256..7907abc 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -9,3 +9,4 @@ 276 auto rw metadata 284 auto 307 auto quick +309 auto After fixing the test #'s it worked and looks reasonable. Reviewed-by: Josef Bacik jba...@fusionio.com Thanks, Josef -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More
Re: [PATCH 3/3] Btrfs: fix several potential problems in copy_nocow_pages_for_inode
On thu, 05 Sep 2013 16:27:44 +0200, Stefan Behrens wrote: On Thu, 27 Jun 2013 18:51:00 +0800, Miao Xie wrote: - It makes no sense that we deal with a inode in the dead tree. This caused that the replace procedure was not dealing with free space cache entries anymore (which have btrfs_root_refs() == 0). I accidentally fixed it as a side-effect of Btrf: cleanup: don't check for root_refs == 0 twice and intentionally fixed it with Btrfs: eliminate the exceptional root_tree refs=0 (which is not yet sent). Thanks for your fix. - fix the race between dio and page copy by waiting the dio completion This causes lockdep issues which I've seen once after running './check -g all' followed by './check btrfs/005' during the 005 run, and on a second box once while running the btrfs xfstests 001 up to 011 during the xfstest 011 run, both boxes were running the latest btrfs-next plus the patch Btrfs: eliminate the exceptional root_tree refs=0 (but I do not think that the latter patch matters). I'm unable to reproduce this lockdep warning, but it seems to make sense, see below. ### scrub.c: static void copy_nocow_pages_worker(struct btrfs_work *work) { ... trans = btrfs_join_transaction(root); It seems btrfs_join_transaction() here is used to prevent the transaction from being committed, but we wait for the running scrubber and pause the scrubber before the transaction is committed, so do we need btrfs_join_transaction() here? Thanks Miao ... ret = iterate_inodes_from_logical(logical, fs_info, path, copy_nocow_pages_for_inode, nocow_ctx); ... btrfs_end_transaction(trans, root); ... } static int copy_nocow_pages_for_inode(u64 inum, u64 offset, u64 root, void *ctx) { ... /* Avoid truncate/dio/punch hole.. */ mutex_lock(inode-i_mutex); inode_dio_wait(inode); ... mutex_unlock(inode-i_mutex); ... } ### file.c: int btrfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync) { ... mutex_lock(inode-i_mutex); ... trans = btrfs_start_transaction(root, 0); ... mutex_unlock(inode-i_mutex); ... ret = btrfs_end_transaction(trans, root); ... } [ INFO: possible circular locking dependency detected ] 3.10.0+ #5 Not tainted --- xfs_io/30404 is trying to acquire lock: (sb_internal){.+.+..}, at: [a00abad6] start_transaction+0x296/0x4f0 [btrfs] but task is already holding lock: (sb-s_type-i_mutex_key#11){+.+.+.}, at: [a00bd029] btrfs_sync_file+0xb9/0x2c0 [btrfs] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: - #1 (sb-s_type-i_mutex_key#11){+.+.+.}: [810e564a] lock_acquire+0x8a/0x120 [81993f13] mutex_lock_nested+0x73/0x390 [a01005d5] copy_nocow_pages_for_inode+0x105/0x2b0 [btrfs] [a010941e] iterate_extent_inodes+0x1ce/0x300 [btrfs] [a01095f7] iterate_inodes_from_logical+0xa7/0xb0 [btrfs] [a00fffeb] copy_nocow_pages_worker+0x9b/0x160 [btrfs] [a00d9b1f] worker_loop+0x13f/0x5b0 [btrfs] [810abd36] kthread+0xd6/0xe0 [8199f66c] ret_from_fork+0x7c/0xb0 - #0 (sb_internal){.+.+..}: [810e4f42] __lock_acquire+0x1d12/0x1e10 [810e564a] lock_acquire+0x8a/0x120 [8119d7ab] __sb_start_write+0xdb/0x1c0 [a00abad6] start_transaction+0x296/0x4f0 [btrfs] [a00ac0e6] btrfs_start_transaction+0x16/0x20 [btrfs] [a00bd0a9] btrfs_sync_file+0x139/0x2c0 [btrfs] [811c9848] do_fsync+0x58/0x80 [811c9adb] SyS_fsync+0xb/0x10 [8199f712] system_call_fastpath+0x16/0x1b other info that might help us debug this: Possible unsafe locking scenario: CPU0CPU1 lock(sb-s_type-i_mutex_key#11); lock(sb_internal); lock(sb-s_type-i_mutex_key#11); lock(sb_internal); *** DEADLOCK *** 1 lock held by xfs_io/30404: #0: (sb-s_type-i_mutex_key#11){+.+.+.}, at: [a00bd029] btrfs_sync_file+0xb9/0x2c0 [btrfs] stack backtrace: CPU: 3 PID: 30404 Comm: xfs_io Not tainted 3.10.0+ #5 Hardware name: Supermicro X9SCL/X9SCM/X9SCL/X9SCM, BIOS 2.0b 09/17/2012 865d9790 880804ef5bb8 8198eaba 880804ef5c08 81989a92 03d0 880804ef5c98 880804d786d0 880804d78708 880804d786d0 880804d78000 Call Trace: [8198eaba] dump_stack+0x19/0x1b [81989a92] print_circular_bug+0x1fe/0x20f [810e4f42] __lock_acquire+0x1d12/0x1e10 [810e564a] lock_acquire+0x8a/0x120
Re: [PATCH] Btrfs: eliminate the exceptional root_tree refs=0
On thu, 5 Sep 2013 16:58:43 +0200, Stefan Behrens wrote: The fact that btrfs_root_refs() returned 0 for the tree_root caused bugs in the past, therefore it is set to 1 with this patch and (hopefully) all affected code is adapted to this change. I verified this change by temporarily adding WARN_ON() checks everywhere where btrfs_root_refs() is used, checking whether the logic of the code is changed by btrfs_root_refs() returning 1 instead of 0 for root-root_key.objectid == BTRFS_ROOT_TREE_OBJECTID. With these added checks, I ran the xfstests './check -g auto'. The two roots chunk_root and log_root_tree that are only referenced by the superblock and the log_roots below the log_root_tree still have btrfs_root_refs() == 0, only the tree_root is changed. Signed-off-by: Stefan Behrens sbehr...@giantdisaster.de --- fs/btrfs/disk-io.c | 1 + fs/btrfs/inode-map.c | 3 +-- fs/btrfs/inode.c | 21 - 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index fa8b2c6..ffc3e43 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2667,6 +2667,7 @@ retry_root_backup: btrfs_set_root_node(tree_root-root_item, tree_root-node); tree_root-commit_root = btrfs_root_node(tree_root); + btrfs_set_root_refs(tree_root-root_item, 1); location.objectid = BTRFS_EXTENT_TREE_OBJECTID; location.type = BTRFS_ROOT_ITEM_KEY; diff --git a/fs/btrfs/inode-map.c b/fs/btrfs/inode-map.c index 2c66ddb..d11e1c6 100644 --- a/fs/btrfs/inode-map.c +++ b/fs/btrfs/inode-map.c @@ -412,8 +412,7 @@ int btrfs_save_ino_cache(struct btrfs_root *root, return 0; /* Don't save inode cache if we are deleting this root */ - if (btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) + if (btrfs_root_refs(root-root_item) == 0) return 0; if (!btrfs_test_opt(root, INODE_MAP_CACHE)) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 26992ee..7d0ef55 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -4472,8 +4472,10 @@ void btrfs_evict_inode(struct inode *inode) trace_btrfs_inode_evict(inode); truncate_inode_pages(inode-i_data, 0); - if (inode-i_nlink (btrfs_root_refs(root-root_item) != 0 || -btrfs_is_free_space_inode(inode))) + if (inode-i_nlink + ((btrfs_root_refs(root-root_item) != 0 + root-root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) || + btrfs_is_free_space_inode(inode))) if (inode-i_nlink btrfs_root_refs(root-root_item)) { } is OK, because with this patch, the refs of the free space inodes' root(tree root) should be 1. For the ino cache inodes' root(fs/file root), if the root is dead, we can drop the ino cache inode safely. And if the root is not dead, the refs should be 0, we will skip the drop. goto no_delete; if (is_bad_inode(inode)) { @@ -4490,7 +4492,8 @@ void btrfs_evict_inode(struct inode *inode) } if (inode-i_nlink 0) { - BUG_ON(btrfs_root_refs(root-root_item) != 0); + BUG_ON(btrfs_root_refs(root-root_item) != 0 +root-root_key.objectid != BTRFS_ROOT_TREE_OBJECTID); This change is unnecessary because the refs of the root tree is 1 now. goto no_delete; } @@ -4731,14 +4734,7 @@ static void inode_tree_del(struct inode *inode) } spin_unlock(root-inode_lock); - /* - * Free space cache has inodes in the tree root, but the tree root has a - * root_refs of 0, so this could end up dropping the tree root as a - * snapshot, so we need the extra !root-fs_info-tree_root check to - * make sure we don't drop it. - */ - if (empty btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) { + if (empty btrfs_root_refs(root-root_item) == 0) { synchronize_srcu(root-fs_info-subvol_srcu); spin_lock(root-inode_lock); empty = RB_EMPTY_ROOT(root-inode_tree); @@ -7872,8 +7868,7 @@ int btrfs_drop_inode(struct inode *inode) return 1; /* the snap/subvol tree is on deleting */ - if (btrfs_root_refs(root-root_item) == 0 - root != root-fs_info-tree_root) + if (btrfs_root_refs(root-root_item) == 0) return 1; else return generic_drop_inode(inode); The others is OK. Thanks Miao -- 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