[PATCH v2 0/3] btrfs-progs: prevent mkfs from aborting with small volume

2013-09-05 Thread Hidetoshi Seto
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

2013-09-05 Thread Hidetoshi Seto
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

2013-09-05 Thread Hidetoshi Seto
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

2013-09-05 Thread Hidetoshi Seto
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

2013-09-05 Thread Stefan Behrens
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

2013-09-05 Thread Stefan Behrens
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

2013-09-05 Thread Wang Shilong

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

2013-09-05 Thread Gui Hecheng
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

2013-09-05 Thread Gui Hecheng
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

2013-09-05 Thread Wang Shilong

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

2013-09-05 Thread Stefan Behrens
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

2013-09-05 Thread Wang Shilong

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

2013-09-05 Thread Wang Shilong

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

2013-09-05 Thread Stefan Behrens
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

2013-09-05 Thread Тимофей Титовец
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

2013-09-05 Thread Stefan Behrens
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

2013-09-05 Thread Roman Mamedov
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

2013-09-05 Thread Hugo Mills
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

2013-09-05 Thread Roman Mamedov
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

2013-09-05 Thread Hugo Mills
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

2013-09-05 Thread Eric Sandeen
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

2013-09-05 Thread Josef Bacik
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

2013-09-05 Thread Josef Bacik
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

2013-09-05 Thread Miao Xie
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

2013-09-05 Thread Miao Xie
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