Re: btrfs-convert won't convert ext* - No valid Btrfs found on /dev/sdb1

[Daniel]

Hello Josef,

Josef Bacik jbacik at fusionio.com writes:

 
 On Thu, Sep 05, 2013 at 10:45:23AM -0500, Eric Sandeen wrote:

[...]

  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.  ;),

Is there any progress on this or can I help with solving this somehow?
 
 Josef

Daniel

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

[Wang Shilong]

Hello Daniel,

On 10/11/2013 03:52 PM, Daniel wrote:

Hello Josef,

Josef Bacik jbacik at fusionio.com writes:


On Thu, Sep 05, 2013 at 10:45:23AM -0500, Eric Sandeen wrote:

[...]


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.  ;),

Is there any progress on this or can I help with solving this somehow?

This problem has been fixed , but it did not come into Chris's branch.
But it did come into david's integration branches.

http://github.com/kdave/btrfs-progs.git integration-20131008

Thanks,
Wang
  

Josef

Daniel

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

[PATCH] Btrfs: fixup error path in __btrfs_inc_extent_ref

[Liu Bo]

When we fail to add a reference after a non-inline insertion by some reasons,
eg. ENOSPC, we'll abort the transaction, but we don't return this error to
the caller who has to walk around again to find something wrong, that's
unnecessary.

Also fixup other error paths to keep it simple.

Signed-off-by: Liu Bo bo.li@oracle.com
---
 fs/btrfs/extent-tree.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index d58bef1..680f4df 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1979,7 +1979,6 @@ static int __btrfs_inc_extent_ref(struct 
btrfs_trans_handle *trans,
struct btrfs_extent_item *item;
u64 refs;
int ret;
-   int err = 0;
 
path = btrfs_alloc_path();
if (!path)
@@ -1992,14 +1991,9 @@ static int __btrfs_inc_extent_ref(struct 
btrfs_trans_handle *trans,
   path, bytenr, num_bytes, parent,
   root_objectid, owner, offset,
   refs_to_add, extent_op);
-   if (ret == 0)
+   if (ret != -EAGAIN)
goto out;
 
-   if (ret != -EAGAIN) {
-   err = ret;
-   goto out;
-   }
-
leaf = path-nodes[0];
item = btrfs_item_ptr(leaf, path-slots[0], struct btrfs_extent_item);
refs = btrfs_extent_refs(leaf, item);
@@ -2021,7 +2015,7 @@ static int __btrfs_inc_extent_ref(struct 
btrfs_trans_handle *trans,
btrfs_abort_transaction(trans, root, ret);
 out:
btrfs_free_path(path);
-   return err;
+   return ret;
 }
 
 static int run_delayed_data_ref(struct btrfs_trans_handle *trans,
-- 
1.8.1.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: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Stefan Behrens]

On Fri, 11 Oct 2013 09:13:24 +0800, Wang Shilong wrote:
 On 10/11/2013 01:40 AM, Ilya Dryomov wrote:
 
 I have a question in my mind.
 
 Can we reach a state that there is operation in progress when filesystem
 has been readonly?If we do cancel operations on a ro filesystem, we should
 get No operations in progress .

Well, it's arguable what ro means. No write to the devices at all? Or
replay log on mount (which means to write to the filesystem), but no
write access in addition to that? Or allow filesystem internal things to
be modified and written to disk, like it was done when the balance or
replace control items were modified on disk when the cancelling was
requested?

In any case, don't make it more complicated then necessary IMO, and
return EROFS if someone calls cancel on a ro filesystem regardless of
the state of the operation. It only adds errors to try to distuingish
such things and is of no benefit for anybody IMHO.


 For both balance and replace, cancelling involves changing the on-disk
 state and committing a transaction, which is not a good thing to do on
 read-only filesystems.

 Cc: Stefan Behrens sbehr...@giantdisaster.de
 Signed-off-by: Ilya Dryomov idryo...@gmail.com
 ---
   fs/btrfs/dev-replace.c |3 +++
   fs/btrfs/volumes.c |3 +++
   2 files changed, 6 insertions(+)

 diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
 index 9efb94e..98df261 100644
 --- a/fs/btrfs/dev-replace.c
 +++ b/fs/btrfs/dev-replace.c
 @@ -650,6 +650,9 @@ static u64 __btrfs_dev_replace_cancel(struct
 btrfs_fs_info *fs_info)
   u64 result;
   int ret;
   +if (fs_info-sb-s_flags  MS_RDONLY)
 +return -EROFS;
 +
   mutex_lock(dev_replace-lock_finishing_cancel_unmount);
   btrfs_dev_replace_lock(dev_replace);
   switch (dev_replace-replace_state) {
 diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
 index a306db9..2630f38 100644
 --- a/fs/btrfs/volumes.c
 +++ b/fs/btrfs/volumes.c
 @@ -3424,6 +3424,9 @@ int btrfs_pause_balance(struct btrfs_fs_info
 *fs_info)
 int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
   {
 +if (fs_info-sb-s_flags  MS_RDONLY)
 +return -EROFS;
 +
   mutex_lock(fs_info-balance_mutex);
   if (!fs_info-balance_ctl) {
   mutex_unlock(fs_info-balance_mutex);
 

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Remco Hosman - Yerf-IT]

Op 11-10-2013 11:23, Stefan Behrens schreef:

On Fri, 11 Oct 2013 09:13:24 +0800, Wang Shilong wrote:

On 10/11/2013 01:40 AM, Ilya Dryomov wrote:

I have a question in my mind.

Can we reach a state that there is operation in progress when filesystem
has been readonly?If we do cancel operations on a ro filesystem, we should
get No operations in progress .

Well, it's arguable what ro means. No write to the devices at all? Or
replay log on mount (which means to write to the filesystem), but no
write access in addition to that? Or allow filesystem internal things to
be modified and written to disk, like it was done when the balance or
replace control items were modified on disk when the cancelling was
requested?

In any case, don't make it more complicated then necessary IMO, and
return EROFS if someone calls cancel on a ro filesystem regardless of
the state of the operation. It only adds errors to try to distuingish
such things and is of no benefit for anybody IMHO.


just my 2 cents:

I once had to recover a ext3 filesystem from a device that would crash 
if you write anything beyond 2T. The problem is that there was an entry 
in the journal telling the OS to do just that. so the device would just 
crash every time out mount the filesystem. even in RO mode.


The manual speaks about a 'skip journal replay' option, but it was never 
implemented.

kernel source was something like:
case: SKIP_JOURNAL_REPLAY:
return error;


The result: there was no way to mount the filesystem, even in RO mode. i 
endedup dd'ing the whole thing to another device, and then mounting the 
resulting image. it took a very long time!


i would expect a RO mount never to write anything to a filesystem. not 
even replay a journal (or a seperate option for that).
Its possible that the device is not writable at all, if its a snapshot 
or a RO iscsi device of some kind.


Remco


For both balance and replace, cancelling involves changing the on-disk
state and committing a transaction, which is not a good thing to do on
read-only filesystems.

Cc: Stefan Behrens sbehr...@giantdisaster.de
Signed-off-by: Ilya Dryomov idryo...@gmail.com
---
   fs/btrfs/dev-replace.c |3 +++
   fs/btrfs/volumes.c |3 +++
   2 files changed, 6 insertions(+)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 9efb94e..98df261 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -650,6 +650,9 @@ static u64 __btrfs_dev_replace_cancel(struct
btrfs_fs_info *fs_info)
   u64 result;
   int ret;
   +if (fs_info-sb-s_flags  MS_RDONLY)
+return -EROFS;
+
   mutex_lock(dev_replace-lock_finishing_cancel_unmount);
   btrfs_dev_replace_lock(dev_replace);
   switch (dev_replace-replace_state) {
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a306db9..2630f38 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3424,6 +3424,9 @@ int btrfs_pause_balance(struct btrfs_fs_info
*fs_info)
 int btrfs_cancel_balance(struct btrfs_fs_info *fs_info)
   {
+if (fs_info-sb-s_flags  MS_RDONLY)
+return -EROFS;
+
   mutex_lock(fs_info-balance_mutex);
   if (!fs_info-balance_ctl) {
   mutex_unlock(fs_info-balance_mutex);

--
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] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Ilya Dryomov]

On Fri, Oct 11, 2013 at 4:13 AM, Wang Shilong
wangsl.f...@cn.fujitsu.com wrote:
 On 10/11/2013 01:40 AM, Ilya Dryomov wrote:

 I have a question in my mind.

 Can we reach a state that there is operation in progress when filesystem
 has been readonly?If we do cancel operations on a ro filesystem, we should
 get No operations in progress .

So if I understand you correctly you are saying that we should return
the Not in progress error code if the filesystem is mounted ro and
there is nothing to cancel.  I actually did think about it, but decided
against it because one can extend this argument to starting commands
with something like If I order a start on a read-only fs and that
operation has already been started, I should get EINPROGRESS., and
that's not what we are currently doing.

Thanks,

Ilya
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Ilya Dryomov]

On Fri, Oct 11, 2013 at 12:28 PM, Remco Hosman - Yerf-IT
re...@yerf-it.nl wrote:
 i would expect a RO mount never to write anything to a filesystem. not even
 replay a journal (or a seperate option for that).
 Its possible that the device is not writable at all, if its a snapshot or a
 RO iscsi device of some kind.

I agree, and that's exactly the point of this patch.

Thanks,

Ilya
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Roman Mamedov]

On Fri, 11 Oct 2013 11:23:04 +0200
Stefan Behrens sbehr...@giantdisaster.de wrote:

 On Fri, 11 Oct 2013 09:13:24 +0800, Wang Shilong wrote:
  On 10/11/2013 01:40 AM, Ilya Dryomov wrote:
  
  I have a question in my mind.
  
  Can we reach a state that there is operation in progress when filesystem
  has been readonly?If we do cancel operations on a ro filesystem, we should
  get No operations in progress .
 
 Well, it's arguable what ro means. No write to the devices at all?

If I had an FS image and mounted it as -o loop,ro I'd simply expect md5sum of
that image to match before mount and after unmount, i.e. no writes at all.
Really, how can one argue with what read only means? If it will mean
something else than a complete absence of writes, then how can we mount
devices or FS images to do forensics, etc? Or do a recovery from a difficult
corruption or try to debug an FS crash.

-- 
With respect,
Roman


signature.asc
Description: PGP signature

Re: [PATCH] Btrfs: disallow 'btrfs {balance,replace} cancel' on ro mounts

[Hugo Mills]

On Fri, Oct 11, 2013 at 03:35:46PM +0600, Roman Mamedov wrote:
 On Fri, 11 Oct 2013 11:23:04 +0200
 Stefan Behrens sbehr...@giantdisaster.de wrote:
 
  On Fri, 11 Oct 2013 09:13:24 +0800, Wang Shilong wrote:
   On 10/11/2013 01:40 AM, Ilya Dryomov wrote:
   
   I have a question in my mind.
   
   Can we reach a state that there is operation in progress when filesystem
   has been readonly?If we do cancel operations on a ro filesystem, we should
   get No operations in progress .
  
  Well, it's arguable what ro means. No write to the devices at all?
 
 If I had an FS image and mounted it as -o loop,ro I'd simply expect md5sum of
 that image to match before mount and after unmount, i.e. no writes at all.
 Really, how can one argue with what read only means? If it will mean
 something else than a complete absence of writes, then how can we mount
 devices or FS images to do forensics, etc? Or do a recovery from a difficult
 corruption or try to debug an FS crash.

   I think the issue here is that with ext3,4 the FS data structures
can be in an inconsistent (i.e. broken) state unless you do the
journal replay, so you're left with the choice of replay the journal
and have a working filesystem that you can't write to, or don't
modify anything, and have a useless filesystem with errors you could
fix with the information in the journal.

   Fortunately, we don't (well, shouldn't) have that issue in btrfs --
if we don't replay the log, we've still got a consistent FS, so it's
much easier conceptually to mount an FS read-only (with the proviso
that a RO mount may be missing some data that was saved earlier, but
you'll get that back if you allow the log reply with a rw mount
later).

   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
 --- Gentlemen!  You can't fight here! This is the War Room! --- 


signature.asc
Description: Digital signature

Re: [PATCH v2] Btrfs: improve inode hash function/inode lookup

[David Sterba]

On Sun, Oct 06, 2013 at 10:22:33PM +0100, Filipe David Borba Manana wrote:
 2) On 32 bits machines. Th VFS hash values are unsigned longs, which
are 32 bits wide on 32 bits machines, and the inode (objectid)
numbers are 64 bits unsigned integers. We simply cast the inode
numbers to hash values, which means that for all inodes with the
same 32 bits lower half, the same hash bucket is used for all of
them. For example, all inodes with a number (objectid) between
0x___ and 0x___ will end up in
the same hash table bucket.

Well, inode number that does not fit into 32 bits on a 32 bit machine
causes other problems. And subvolume ids that do not fit into 32 bits
cannot be stored in radix tree.

It would be safer to refuse creating/accessing inode/subvolume with
nubmer that does not fit into 32bits.

david
--
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 1/2] btrfs-progs: device scan use BTRFS_SCAN_LBLKID by default

[Anand Jain]

with this patch, BTRFS_SCAN_LBLKID (which leverages lblkid
to look for btrfs disks) would be the default scan method
to look for the btrfs disks. And thus the output as seen
in the latest btrfs fi show and btrfs fi show -m for the
mounted disks will have the consistent disks path.
(it was inconsistent (across disks) because btrfs dev scan
provided a different path from the mount command eg. below)

devid1 size 1.98GiB used 435.00MiB path /dev/mapper/mpatha
devid2 size 2.00GiB used 415.00MiB path /dev/dm-1

Signed-off-by: Anand Jain anand.j...@oracle.com
---
 cmds-device.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 7cfc347..1315918 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -189,7 +189,7 @@ static const char * const cmd_scan_dev_usage[] = {
 static int cmd_scan_dev(int argc, char **argv)
 {
int i, fd, e;
-   int where = BTRFS_SCAN_PROC;
+   int where = BTRFS_SCAN_LBLKID;
int devstart = 1;
 
if( argc  1  !strcmp(argv[1],--all-devices)){
-- 
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 2/2] btrfs-progs: use the marco BTRFS_UPDATE_KERNEL where needed

[Anand Jain]

Signed-off-by: Anand Jain anand.j...@oracle.com
---
 cmds-device.c |2 +-
 utils.c   |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 1315918..6f32356 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -203,7 +203,7 @@ static int cmd_scan_dev(int argc, char **argv)
if(argc=devstart){
int ret;
printf(Scanning for Btrfs filesystems\n);
-   ret = scan_for_btrfs(where, 1);
+   ret = scan_for_btrfs(where, BTRFS_UPDATE_KERNEL);
if (ret){
fprintf(stderr, ERROR: error %d while scanning\n, 
ret);
return 1;
diff --git a/utils.c b/utils.c
index 2f1d54f..f324147 100644
--- a/utils.c
+++ b/utils.c
@@ -961,7 +961,7 @@ int check_mounted_where(int fd, const char *file, char 
*where, int size,
 
/* scan other devices */
if (is_btrfs  total_devs  1) {
-   if((ret = btrfs_scan_for_fsid(0)))
+   if((ret = btrfs_scan_for_fsid(!BTRFS_UPDATE_KERNEL)))
return ret;
}
 
-- 
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] Btrfs: fail device statistic reset on read-only filesystem

[Stefan Behrens]

Currently the attempt to reset the device statistics with
'btrfs device stat -z ...' on read-only filesystems does not return an
error. The statistics that are hold in main memory are reset but the
statistics that are stored in the filesystem are not reset.

Fix it by returning EROFS in this case.

This is the reproducer:

 #!/bin/sh
TEST_DEV1=/dev/sdz1
TEST_DEV2=/dev/sdz2
TEST_MNT=/mnt
echo 0 25165824 linear $TEST_DEV1 0 | dmsetup create foom
echo 0 25165824 linear $TEST_DEV2 0 | dmsetup create foon
mkfs.btrfs -f -d raid1 -m raid1 /dev/mapper/foom /dev/mapper/foon
mount /dev/mapper/foom $TEST_MNT
 # switch to I/O error mode:
echo 0 25165824 error | dmsetup reload foon
dmsetup resume foon
 # cause I/O errors:
touch ${TEST_MNT}/foo
sync
 # switch dm back to I/O good mode:
echo 0 25165824 linear $TEST_DEV1 0 | dmsetup reload foon
dmsetup resume foon
umount ${TEST_MNT}
btrfs dev scan
mount /dev/mapper/foom ${TEST_MNT} -o ro
 # will report counters != 0, should fail with EROFS
btrfs device stat -z ${TEST_MNT}
 # will report counters == 0:
btrfs device stat ${TEST_MNT}
umount ${TEST_MNT}
mount /dev/mapper/foom ${TEST_MNT} -o ro
 # will report counters != 0, i.e., the 'btrfs device stat -z' failed to
 # clear the counters on disk, only the counters in main memory had been
 # cleared:
btrfs device stat ${TEST_MNT}
umount ${TEST_MNT}
dmsetup remove foom; dmsetup remove foon

Signed-off-by: Stefan Behrens sbehr...@giantdisaster.de
---
 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index fe0f2ef..646d10d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6250,6 +6250,8 @@ int btrfs_get_dev_stats(struct btrfs_root *root,
   btrfs: get dev_stats failed, not yet valid\n);
return -ENODEV;
} else if (stats-flags  BTRFS_DEV_STATS_RESET) {
+   if (root-fs_info-sb-s_flags  MS_RDONLY)
+   return -EROFS;
for (i = 0; i  BTRFS_DEV_STAT_VALUES_MAX; i++) {
if (stats-nr_items  i)
stats-values[i] =
-- 
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: [PATCH v5] btrfs: Fix memory leakage in the tree-log.c

[Stefan Behrens]

On Thu, 10 Oct 2013 19:11:22 -0300, Geyslan G. Bem wrote:
 In add_inode_ref() function:
 
 Initializes local pointers.
 
 Reduces the logical condition with the __add_inode_ref() return
 value by using only one 'goto out'.
 
 Centralizes the exiting, ensuring the freeing of all used memory.
 
 Signed-off-by: Geyslan G. Bem geys...@gmail.com

The patch looks correct to me, but there are some nitpicking style issues.


 ---
  fs/btrfs/tree-log.c | 36 ++--
  1 file changed, 22 insertions(+), 14 deletions(-)
 
 diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
 index 79f057c..beb41b0 100644
 --- a/fs/btrfs/tree-log.c
 +++ b/fs/btrfs/tree-log.c
 @@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
 struct extent_buffer *eb, int slot,
 struct btrfs_key *key)
  {
 - struct inode *dir;
 - struct inode *inode;
 + struct inode *dir = NULL;
 + struct inode *inode = NULL;
   unsigned long ref_ptr;
   unsigned long ref_end;
 - char *name;
 + char *name = NULL;
   int namelen;
   int ret;
   int search_done = 0;
 @@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
* care of the rest
*/
   dir = read_one_inode(root, parent_objectid);
 - if (!dir)
 - return -ENOENT;
 + if (!dir) {
 + ret = -ENOENT;
 + goto out;
 + }
  
   inode = read_one_inode(root, inode_objectid);
   if (!inode) {
 - iput(dir);
 - return -EIO;
 + ret = -EIO;
 + goto out;
   }
  
   while (ref_ptr  ref_end) {
 @@ -1169,14 +1171,17 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
*/
   if (!dir)
   dir = read_one_inode(root, parent_objectid);
 - if (!dir)
 - return -ENOENT;
 + if (!dir) {
 + ret = -ENOENT;
 + goto out;
 + }
   } else {
   ret = ref_get_fields(eb, ref_ptr, namelen, name,
ref_index);
   }
   if (ret)
 - return ret;
 + goto out;
 +

This additional empty line is not required, we would have two empty
lines in a row.


  
   /* if we already have a perfect match, we're done */
   if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
 @@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
 parent_objectid,
 ref_index, name, namelen,
 search_done);
 - if (ret == 1) {
 - ret = 0;
 +

This empty line is also not required. You know, this hurts on regular
80x24 terminals. Do you get more than 24 lines on your display?
I thought there was a rule for this in Documentation/CodingStyle, but
there isn't. Therefore it's up to you. But the two empty lines above are
definitely not wanted.


 + if (ret) {
 + if (ret == 1)
 + ret--;

I don't see a reason to change the ret = 0 to a ret--, this is not
an optimization and makes the code less readable.


   goto out;
   }
 - if (ret)
 - goto out;
   }
  
   /* insert our name */
 @@ -1215,6 +1220,8 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
  
   ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
   kfree(name);
 + name = NULL;
 +

Another empty line which is not required for the purpose of this patch.


   if (log_ref_ver) {
   iput(dir);
   dir = NULL;
 @@ -1225,6 +1232,7 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
   ret = overwrite_item(trans, root, path, eb, slot, key);
  out:
   btrfs_release_path(path);
 + kfree(name);
   iput(dir);
   iput(inode);
   return ret;
 


--
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 v5] btrfs: Fix memory leakage in the tree-log.c

[Geyslan Gregório Bem]

2013/10/11 Stefan Behrens sbehr...@giantdisaster.de:
 On Thu, 10 Oct 2013 19:11:22 -0300, Geyslan G. Bem wrote:
 In add_inode_ref() function:

 Initializes local pointers.

 Reduces the logical condition with the __add_inode_ref() return
 value by using only one 'goto out'.

 Centralizes the exiting, ensuring the freeing of all used memory.

 Signed-off-by: Geyslan G. Bem geys...@gmail.com

 The patch looks correct to me, but there are some nitpicking style issues.


 ---
  fs/btrfs/tree-log.c | 36 ++--
  1 file changed, 22 insertions(+), 14 deletions(-)

 diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
 index 79f057c..beb41b0 100644
 --- a/fs/btrfs/tree-log.c
 +++ b/fs/btrfs/tree-log.c
 @@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
 struct extent_buffer *eb, int slot,
 struct btrfs_key *key)
  {
 - struct inode *dir;
 - struct inode *inode;
 + struct inode *dir = NULL;
 + struct inode *inode = NULL;
   unsigned long ref_ptr;
   unsigned long ref_end;
 - char *name;
 + char *name = NULL;
   int namelen;
   int ret;
   int search_done = 0;
 @@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
* care of the rest
*/
   dir = read_one_inode(root, parent_objectid);
 - if (!dir)
 - return -ENOENT;
 + if (!dir) {
 + ret = -ENOENT;
 + goto out;
 + }

   inode = read_one_inode(root, inode_objectid);
   if (!inode) {
 - iput(dir);
 - return -EIO;
 + ret = -EIO;
 + goto out;
   }

   while (ref_ptr  ref_end) {
 @@ -1169,14 +1171,17 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
*/
   if (!dir)
   dir = read_one_inode(root, parent_objectid);
 - if (!dir)
 - return -ENOENT;
 + if (!dir) {
 + ret = -ENOENT;
 + goto out;
 + }
   } else {
   ret = ref_get_fields(eb, ref_ptr, namelen, name,
ref_index);
   }
   if (ret)
 - return ret;
 + goto out;
 +

 This additional empty line is not required, we would have two empty
 lines in a row.

Ok, I'll remove it.




   /* if we already have a perfect match, we're done */
   if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
 @@ -1196,12 +1201,12 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
 parent_objectid,
 ref_index, name, namelen,
 search_done);
 - if (ret == 1) {
 - ret = 0;
 +

 This empty line is also not required. You know, this hurts on regular
 80x24 terminals. Do you get more than 24 lines on your display?
 I thought there was a rule for this in Documentation/CodingStyle, but
 there isn't. Therefore it's up to you. But the two empty lines above are
 definitely not wanted.


Yes, I get more than 24 lines. But no problem, I know about the Coding Style.
This issue will be fixed.


 + if (ret) {
 + if (ret == 1)
 + ret--;

 I don't see a reason to change the ret = 0 to a ret--, this is not
 an optimization and makes the code less readable.


Really. Using -O2 the code is equal. I'll redo to ret = 0; with the
new conditional scope.


   goto out;
   }
 - if (ret)
 - goto out;
   }

   /* insert our name */
 @@ -1215,6 +1220,8 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,

   ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
   kfree(name);
 + name = NULL;
 +

 Another empty line which is not required for the purpose of this patch.

Ok, it'll be removed.



   if (log_ref_ver) {
   iput(dir);
   dir = NULL;
 @@ -1225,6 +1232,7 @@ static noinline int add_inode_ref(struct 
 btrfs_trans_handle *trans,
   ret = overwrite_item(trans, root, path, eb, slot, key);
  out:
   btrfs_release_path(path);
 + kfree(name);
   iput(dir);
   iput(inode);
   return ret;




Thank you again.

Geyslan G. Bem
--
To unsubscribe from this list: send the line 

[PATCH] Btrfs: init device stats for new devices

[Stefan Behrens]

Device stats are only initialized (read from tree items) on mount.
Trying to read device stats after adding or replacing new devices will
return errors.

btrfs_init_new_device() and btrfs_init_dev_replace_tgtdev() are the two
functions that allocate and initialize new btrfs_device structures after
a filesystem is mounted. They set the device stats to zero by using
kzalloc() which is correct for new devices. The only missing thing was
to declare these stats as being valid (device-dev_stats_valid = 1) and
this patch adds this missing code.

This is the reproducer:

TEST_DEV1=/dev/sdz1
TEST_DEV2=/dev/sdz2
TEST_DEV3=/dev/sdz3
TEST_MNT=/mnt
mkfs.btrfs $TEST_DEV1
mount $TEST_DEV1 $TEST_MNT
btrfs device add $TEST_DEV2 $TEST_MNT
btrfs device stat $TEST_MNT
btrfs replace start -B $TEST_DEV2 $TEST_DEV3 $TEST_MNT
btrfs device stat $TEST_MNT
umount $TEST_MNT

Reported-by: Ondrej Kunc kun...@gmail.com
Signed-off-by: Stefan Behrens sbehr...@giantdisaster.de
---
The idea to fix this issue, the subject line of the patch and parts
of the commit log are reused from a patch that Zach Brown has sent.

 fs/btrfs/volumes.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 646d10d..9837439 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2041,6 +2041,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char 
*device_path)
device-in_fs_metadata = 1;
device-is_tgtdev_for_dev_replace = 0;
device-mode = FMODE_EXCL;
+   device-dev_stats_valid = 1;
set_blocksize(device-bdev, 4096);
 
if (seeding_dev) {
@@ -2208,6 +2209,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_root 
*root, char *device_path,
device-in_fs_metadata = 1;
device-is_tgtdev_for_dev_replace = 1;
device-mode = FMODE_EXCL;
+   device-dev_stats_valid = 1;
set_blocksize(device-bdev, 4096);
device-fs_devices = fs_info-fs_devices;
list_add(device-dev_list, fs_info-fs_devices-devices);
-- 
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: [PATCH] Btrfs: fail device statistic reset on read-only filesystem

[Josef Bacik]

On Fri, Oct 11, 2013 at 02:18:23PM +0200, Stefan Behrens wrote:
 Currently the attempt to reset the device statistics with
 'btrfs device stat -z ...' on read-only filesystems does not return an
 error. The statistics that are hold in main memory are reset but the
 statistics that are stored in the filesystem are not reset.
 
 Fix it by returning EROFS in this case.
 
 This is the reproducer:
 
  #!/bin/sh
 TEST_DEV1=/dev/sdz1
 TEST_DEV2=/dev/sdz2
 TEST_MNT=/mnt
 echo 0 25165824 linear $TEST_DEV1 0 | dmsetup create foom
 echo 0 25165824 linear $TEST_DEV2 0 | dmsetup create foon
 mkfs.btrfs -f -d raid1 -m raid1 /dev/mapper/foom /dev/mapper/foon
 mount /dev/mapper/foom $TEST_MNT
  # switch to I/O error mode:
 echo 0 25165824 error | dmsetup reload foon
 dmsetup resume foon
  # cause I/O errors:
 touch ${TEST_MNT}/foo
 sync
  # switch dm back to I/O good mode:
 echo 0 25165824 linear $TEST_DEV1 0 | dmsetup reload foon
 dmsetup resume foon
 umount ${TEST_MNT}
 btrfs dev scan
 mount /dev/mapper/foom ${TEST_MNT} -o ro
  # will report counters != 0, should fail with EROFS
 btrfs device stat -z ${TEST_MNT}
  # will report counters == 0:
 btrfs device stat ${TEST_MNT}
 umount ${TEST_MNT}
 mount /dev/mapper/foom ${TEST_MNT} -o ro
  # will report counters != 0, i.e., the 'btrfs device stat -z' failed to
  # clear the counters on disk, only the counters in main memory had been
  # cleared:
 btrfs device stat ${TEST_MNT}
 umount ${TEST_MNT}
 dmsetup remove foom; dmsetup remove foon
 

Hey look something else that should go into xfstests,

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] Btrfs: fail device statistic reset on read-only filesystem

[Stefan Behrens]

On Fri, 11 Oct 2013 09:47:33 -0400, Josef Bacik wrote:
 Hey look something else that should go into xfstests,

I don't think so. It's a bug that is there from the very beginning, not
a regression. We can't catch all possible errors (non-regressions) with
xfstests. We would spend all time for writing xfstests, there wouldn't
be any time left to actually add bugs.


--
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: BUG relating to fstrim on btrfs partitions

[Eric Sandeen]

On 10/10/13 6:39 AM, Duncan wrote:
 Mike Audia posted on Thu, 10 Oct 2013 06:20:42 -0400 as excerpted:
 
 I think I found a bug affecting btrfs filesystems and users invoking
 fstrim to discard unused blocks: if I execute a `fstrim -v /` twice, the
 amount trimmed does not change on the 2nd invocation AND it takes just
 as long as the first.  Why do I think this is a bug?  When I do the same
 on an ext4 partition I get different behavior: the output shows 0 B
 trimmed and it does is instantaneously when I run it a 2nd time.  After
 contacting the fstrim developer, he stated that the userspace part
 (fstrim) does only one thing and it is invoke an ioctl (FITRIM); it is
 the job of the filesystem to properly implement this.
 
 This behavior is documented in the fstrim manpage under -v/--verbose:
 
 When [--verbose is] specified fstrim will output the number of bytes
 passed from the filesystem down the block stack to the device for
 potential discard. This number is a maximum discard amount from the
 storage device's perspective, because FITRIM ioctl called repeated
 will keep sending the same sectors for discard repeatedly.

 fstrim will report the same potential discard bytes each time, but
 only sectors which had been written to between the discards would
 actually be discarded by the storage device.
 
 Why ext4 behavior doesn't conform to that fstrim documentation I can't 
 say (except by stating the obvious that the ext4 filesystem 
 implementation of that ioctl obviously does it differently, but why... 
 you'd have to either ask the ext4 folks or read its docs/sources), but 
 given that fstrim documentation, the btrfs behavior is certainly NOTABUG 
 as it's simply conforming to the documentation.

ext4 is conforming just fine.

fstrim will output the number of bytes passed from the filesystem down
the block stack to the device for potential discard.

It reports the number of bytes passed *from the filesystem* to the block
device for discard, not the total range requested by the user.

If the filesystem is clever enough to know that the range in question has
not been written to since the last discard, then it takes no action, and
reports zero bytes.

So it sounds like btrfs doesn't maintain this already discarded state,
and will re-discard unused regions every time fstrim is issued.

Not a bug per se, but not really optimized.

-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: BUG relating to fstrim on btrfs partitions

[Emil Karlson]

 If the filesystem is clever enough to know that the range in question has
 not been written to since the last discard, then it takes no action, and
 reports zero bytes.

File system images can be rewritten on a new media so there is a
drawback to that.

Best Regards
-Emil
--
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: Wait for uuid-tree rebuild task on remount read-only

[Stefan Behrens]

If the user remounts the filesystem read-only while the uuid-tree
scan and rebuild task is still running (this happens once after the
filesystem was mounted with an old kernel, or when forced with the
mount options), the remount should wait on the tasks completion
before setting the filesystem read-only. Otherwise the background
task continues to write to the filesystem which is apparently not
what users expect.

The reproducer:

TEST_DEV=/dev/sdz1
TEST_MNT=/mnt
mkfs.btrfs -f $TEST_DEV
mount $TEST_DEV $TEST_MNT
for i in `seq 5`; do btrfs subvolume create ${TEST_MNT}/$i; done
umount $TEST_MNT
mount $TEST_DEV $TEST_MNT -o rescan_uuid_tree
sleep 1
ps -elf | fgrep '[btrfs-uuid]' | grep -v grep
mount $TEST_DEV $TEST_MNT -o ro,remount
ps -elf | fgrep '[btrfs-uuid]' | grep -v grep
sleep 1
umount $TEST_MNT

Signed-off-by: Stefan Behrens sbehr...@giantdisaster.de
---
 fs/btrfs/super.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2bdaafd..1127d18 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1330,6 +1330,12 @@ static int btrfs_remount(struct super_block *sb, int 
*flags, char *data)
 * this also happens on 'umount -rf' or on shutdown, when
 * the filesystem is busy.
 */
+
+   /* wait for the uuid_scan task to finish */
+   down(fs_info-uuid_tree_rescan_sem);
+   /* avoid complains from lockdep et al. */
+   up(fs_info-uuid_tree_rescan_sem);
+
sb-s_flags |= MS_RDONLY;
 
btrfs_dev_replace_suspend_for_unmount(fs_info);
-- 
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: BUG relating to fstrim on btrfs partitions

[Eric Sandeen]

On 10/11/13 10:14 AM, Emil Karlson wrote:
 If the filesystem is clever enough to know that the range in question has
 not been written to since the last discard, then it takes no action, and
 reports zero bytes.
 
 File system images can be rewritten on a new media so there is a
 drawback to that.

It's in-memory for the mounted filesystem, not on disk.

It checks the EXT4_GROUP_INFO_WAS_TRIMMED_BIT flag stored in bb_state
in the ext4_group_info structure.

So when you mount a dd'd copy, it takes a fresh look, and DTRT.

-Eric

 Best Regards
 -Emil
 

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Btrfs: improve inode hash function/inode lookup

[Filipe David Manana]

On Fri, Oct 11, 2013 at 12:42 PM, David Sterba dste...@suse.cz wrote:
 On Sun, Oct 06, 2013 at 10:22:33PM +0100, Filipe David Borba Manana wrote:
 2) On 32 bits machines. Th VFS hash values are unsigned longs, which
are 32 bits wide on 32 bits machines, and the inode (objectid)
numbers are 64 bits unsigned integers. We simply cast the inode
numbers to hash values, which means that for all inodes with the
same 32 bits lower half, the same hash bucket is used for all of
them. For example, all inodes with a number (objectid) between
0x___ and 0x___ will end up in
the same hash table bucket.

 Well, inode number that does not fit into 32 bits on a 32 bit machine
 causes other problems. And subvolume ids that do not fit into 32 bits
 cannot be stored in radix tree.

 It would be safer to refuse creating/accessing inode/subvolume with
 nubmer that does not fit into 32bits.

Good point David. However it would be a rather radical behaviour change imho.
I think it should be a separate change if there are no objections
against such change.


 david



-- 
Filipe David Manana,

Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: init device stats for new devices

[Zach Brown]

 diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
 index 646d10d..9837439 100644
 --- a/fs/btrfs/volumes.c
 +++ b/fs/btrfs/volumes.c
 @@ -2041,6 +2041,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char 
 *device_path)
   device-in_fs_metadata = 1;
   device-is_tgtdev_for_dev_replace = 0;
   device-mode = FMODE_EXCL;
 + device-dev_stats_valid = 1;
   set_blocksize(device-bdev, 4096);

Doesn't valid mean that the in-memory atomics reflect the counts in the
btree items?  Am I misunderstanding?

Seems like a reproducer would be: get non-zero stats, unmount, mount,
get stats strictly =.

- z
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: init device stats for new devices

[Zach Brown]

 a filesystem is mounted. They set the device stats to zero by using
 kzalloc() which is correct for new devices.

Oh, right, got it :)

- z
--
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: avoid unnecessary scrub workers allocation

[Wang Shilong]

From: Wang Shilong wangsl.f...@cn.fujitsu.com

We only allocate scrub workers if we pass all the necessary
checks, for example, there are no operation in progress.

Besides, move mutex lock protection outside of scrub_workers_get()
/scrub_workers_put().

Signed-off-by: Wang Shilong wangsl.f...@cn.fujitsu.com
---
 fs/btrfs/scrub.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index f21e2df..54e1830 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2784,7 +2784,6 @@ static noinline_for_stack int scrub_workers_get(struct 
btrfs_fs_info *fs_info,
 {
int ret = 0;
 
-   mutex_lock(fs_info-scrub_lock);
if (fs_info-scrub_workers_refcnt == 0) {
if (is_dev_replace)
btrfs_init_workers(fs_info-scrub_workers, scrub, 1,
@@ -2814,21 +2813,17 @@ static noinline_for_stack int scrub_workers_get(struct 
btrfs_fs_info *fs_info,
}
++fs_info-scrub_workers_refcnt;
 out:
-   mutex_unlock(fs_info-scrub_lock);
-
return ret;
 }
 
 static noinline_for_stack void scrub_workers_put(struct btrfs_fs_info *fs_info)
 {
-   mutex_lock(fs_info-scrub_lock);
if (--fs_info-scrub_workers_refcnt == 0) {
btrfs_stop_workers(fs_info-scrub_workers);
btrfs_stop_workers(fs_info-scrub_wr_completion_workers);
btrfs_stop_workers(fs_info-scrub_nocow_workers);
}
WARN_ON(fs_info-scrub_workers_refcnt  0);
-   mutex_unlock(fs_info-scrub_lock);
 }
 
 int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start,
@@ -2889,23 +2884,18 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 
devid, u64 start,
return -EINVAL;
}
 
-   ret = scrub_workers_get(fs_info, is_dev_replace);
-   if (ret)
-   return ret;
 
mutex_lock(fs_info-fs_devices-device_list_mutex);
dev = btrfs_find_device(fs_info, devid, NULL, NULL);
if (!dev || (dev-missing  !is_dev_replace)) {
mutex_unlock(fs_info-fs_devices-device_list_mutex);
-   scrub_workers_put(fs_info);
return -ENODEV;
}
-   mutex_lock(fs_info-scrub_lock);
 
+   mutex_lock(fs_info-scrub_lock);
if (!dev-in_fs_metadata || dev-is_tgtdev_for_dev_replace) {
mutex_unlock(fs_info-scrub_lock);
mutex_unlock(fs_info-fs_devices-device_list_mutex);
-   scrub_workers_put(fs_info);
return -EIO;
}
 
@@ -2916,10 +2906,17 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 
devid, u64 start,
btrfs_dev_replace_unlock(fs_info-dev_replace);
mutex_unlock(fs_info-scrub_lock);
mutex_unlock(fs_info-fs_devices-device_list_mutex);
-   scrub_workers_put(fs_info);
return -EINPROGRESS;
}
btrfs_dev_replace_unlock(fs_info-dev_replace);
+
+   ret = scrub_workers_get(fs_info, is_dev_replace);
+   if (ret) {
+   mutex_unlock(fs_info-scrub_lock);
+   mutex_unlock(fs_info-fs_devices-device_list_mutex);
+   return ret;
+   }
+
sctx = scrub_setup_ctx(dev, is_dev_replace);
if (IS_ERR(sctx)) {
mutex_unlock(fs_info-scrub_lock);
@@ -2955,10 +2952,10 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 
devid, u64 start,
 
mutex_lock(fs_info-scrub_lock);
dev-scrub_device = NULL;
+   scrub_workers_put(fs_info);
mutex_unlock(fs_info-scrub_lock);
 
scrub_free_ctx(sctx);
-   scrub_workers_put(fs_info);
 
return ret;
 }
-- 
1.7.11.7

--
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 v6] btrfs: Fix memory leakage in the tree-log.c

[Geyslan G. Bem]

In add_inode_ref() function:

Initializes local pointers.

Reduces the logical condition with the __add_inode_ref() return
value by using only one 'goto out'.

Centralizes the exiting, ensuring the freeing of all used memory.

Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
 fs/btrfs/tree-log.c | 33 +++--
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..61bb051 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
  struct extent_buffer *eb, int slot,
  struct btrfs_key *key)
 {
-   struct inode *dir;
-   struct inode *inode;
+   struct inode *dir = NULL;
+   struct inode *inode = NULL;
unsigned long ref_ptr;
unsigned long ref_end;
-   char *name;
+   char *name = NULL;
int namelen;
int ret;
int search_done = 0;
@@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
 * care of the rest
 */
dir = read_one_inode(root, parent_objectid);
-   if (!dir)
-   return -ENOENT;
+   if (!dir) {
+   ret = -ENOENT;
+   goto out;
+   }
 
inode = read_one_inode(root, inode_objectid);
if (!inode) {
-   iput(dir);
-   return -EIO;
+   ret = -EIO;
+   goto out;
}
 
while (ref_ptr  ref_end) {
@@ -1169,14 +1171,16 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
 */
if (!dir)
dir = read_one_inode(root, parent_objectid);
-   if (!dir)
-   return -ENOENT;
+   if (!dir) {
+   ret = -ENOENT;
+   goto out;
+   }
} else {
ret = ref_get_fields(eb, ref_ptr, namelen, name,
 ref_index);
}
if (ret)
-   return ret;
+   goto out;
 
/* if we already have a perfect match, we're done */
if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
@@ -1196,12 +1200,11 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
  parent_objectid,
  ref_index, name, namelen,
  search_done);
-   if (ret == 1) {
-   ret = 0;
+   if (ret) {
+   if (ret == 1)
+   ret = 0;
goto out;
}
-   if (ret)
-   goto out;
}
 
/* insert our name */
@@ -1215,6 +1218,7 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
 
ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
kfree(name);
+   name = NULL;
if (log_ref_ver) {
iput(dir);
dir = NULL;
@@ -1225,6 +1229,7 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
ret = overwrite_item(trans, root, path, eb, slot, key);
 out:
btrfs_release_path(path);
+   kfree(name);
iput(dir);
iput(inode);
return ret;
-- 
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: [PATCH v6] btrfs: Fix memory leakage in the tree-log.c

[Stefan Behrens]

On 10/11/2013 20:35, Geyslan G. Bem wrote:

In add_inode_ref() function:

Initializes local pointers.

Reduces the logical condition with the __add_inode_ref() return
value by using only one 'goto out'.

Centralizes the exiting, ensuring the freeing of all used memory.

Signed-off-by: Geyslan G. Bem geys...@gmail.com
---
  fs/btrfs/tree-log.c | 33 +++--
  1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 79f057c..61bb051 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1113,11 +1113,11 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
  struct extent_buffer *eb, int slot,
  struct btrfs_key *key)
  {
-   struct inode *dir;
-   struct inode *inode;
+   struct inode *dir = NULL;
+   struct inode *inode = NULL;
unsigned long ref_ptr;
unsigned long ref_end;
-   char *name;
+   char *name = NULL;
int namelen;
int ret;
int search_done = 0;
@@ -1150,13 +1150,15 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
 * care of the rest
 */
dir = read_one_inode(root, parent_objectid);
-   if (!dir)
-   return -ENOENT;
+   if (!dir) {
+   ret = -ENOENT;
+   goto out;
+   }

inode = read_one_inode(root, inode_objectid);
if (!inode) {
-   iput(dir);
-   return -EIO;
+   ret = -EIO;
+   goto out;
}

while (ref_ptr  ref_end) {
@@ -1169,14 +1171,16 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
 */
if (!dir)
dir = read_one_inode(root, parent_objectid);
-   if (!dir)
-   return -ENOENT;
+   if (!dir) {
+   ret = -ENOENT;
+   goto out;
+   }
} else {
ret = ref_get_fields(eb, ref_ptr, namelen, name,
 ref_index);
}
if (ret)
-   return ret;
+   goto out;

/* if we already have a perfect match, we're done */
if (!inode_in_dir(root, path, btrfs_ino(dir), btrfs_ino(inode),
@@ -1196,12 +1200,11 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
  parent_objectid,
  ref_index, name, namelen,
  search_done);
-   if (ret == 1) {
-   ret = 0;
+   if (ret) {
+   if (ret == 1)
+   ret = 0;
goto out;
}
-   if (ret)
-   goto out;
}

/* insert our name */
@@ -1215,6 +1218,7 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,

ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
kfree(name);
+   name = NULL;
if (log_ref_ver) {
iput(dir);
dir = NULL;
@@ -1225,6 +1229,7 @@ static noinline int add_inode_ref(struct 
btrfs_trans_handle *trans,
ret = overwrite_item(trans, root, path, eb, slot, key);
  out:
btrfs_release_path(path);
+   kfree(name);
iput(dir);
iput(inode);
return ret;



The patch looks really good now. Thanks!

Reviewed-by: Stefan Behrens sbehr...@giantdisaster.de


--
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: add tests for btrfs_get_extent

[Josef Bacik]

I'm going to be removing hole extents in the near future so I wanted to make a
sanity test for btrfs_get_extent to make sure I don't break anything in the
meantime.  This patch just puts btrfs_get_extent through its paces by giving it
a completely unreasonable mapping to look at and make sure it is giving us back
maps that make sense.  Thanks,

Signed-off-by: Josef Bacik jba...@fusionio.com
---
 fs/btrfs/Makefile|   3 +-
 fs/btrfs/ctree.h |   4 +
 fs/btrfs/inode.c |   8 +
 fs/btrfs/super.c |   3 +
 fs/btrfs/tests/btrfs-tests.c |   8 +-
 fs/btrfs/tests/btrfs-tests.h |   5 +
 fs/btrfs/tests/inode-tests.c | 832 +++
 7 files changed, 861 insertions(+), 2 deletions(-)
 create mode 100644 fs/btrfs/tests/inode-tests.c

diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
index cac4f2d..1a44e42 100644
--- a/fs/btrfs/Makefile
+++ b/fs/btrfs/Makefile
@@ -15,4 +15,5 @@ btrfs-$(CONFIG_BTRFS_FS_POSIX_ACL) += acl.o
 btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o
 
 btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \
-   tests/extent-buffer-tests.o tests/btrfs-tests.o tests/extent-io-tests.o
+   tests/extent-buffer-tests.o tests/btrfs-tests.o \
+   tests/extent-io-tests.o tests/inode-tests.o
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2f39806..9f5e1cf 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -4034,5 +4034,9 @@ static inline int btrfs_defrag_cancelled(struct 
btrfs_fs_info *fs_info)
return signal_pending(current);
 }
 
+/* Sanity test specific functions */
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+void btrfs_test_destroy_inode(struct inode *inode);
+#endif
 
 #endif
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d468246..09c950b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7794,6 +7794,14 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
return inode;
 }
 
+#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+void btrfs_test_destroy_inode(struct inode *inode)
+{
+   btrfs_drop_extent_cache(inode, 0, (u64)-1, 0);
+   kmem_cache_free(btrfs_inode_cachep, BTRFS_I(inode));
+}
+#endif
+
 static void btrfs_i_callback(struct rcu_head *head)
 {
struct inode *inode = container_of(head, struct inode, i_rcu);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2bdaafd..db591e8 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1802,6 +1802,9 @@ static int btrfs_run_sanity_tests(void)
if (ret)
goto out;
ret = btrfs_test_extent_io();
+   if (ret)
+   goto out;
+   ret = btrfs_test_inodes();
 out:
btrfs_destroy_test_fs();
return ret;
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index 697d527..757ef00 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -24,11 +24,17 @@
 
 static struct vfsmount *test_mnt = NULL;
 
+static const struct super_operations btrfs_test_super_ops = {
+   .alloc_inode= btrfs_alloc_inode,
+   .destroy_inode  = btrfs_test_destroy_inode,
+};
+
 static struct dentry *btrfs_test_mount(struct file_system_type *fs_type,
   int flags, const char *dev_name,
   void *data)
 {
-   return mount_pseudo(fs_type, btrfs_test:, NULL, NULL, 
BTRFS_TEST_MAGIC);
+   return mount_pseudo(fs_type, btrfs_test:, btrfs_test_super_ops,
+   NULL, BTRFS_TEST_MAGIC);
 }
 
 static struct file_system_type test_type = {
diff --git a/fs/btrfs/tests/btrfs-tests.h b/fs/btrfs/tests/btrfs-tests.h
index e935fe5..3cde5bb 100644
--- a/fs/btrfs/tests/btrfs-tests.h
+++ b/fs/btrfs/tests/btrfs-tests.h
@@ -26,6 +26,7 @@
 int btrfs_test_free_space_cache(void);
 int btrfs_test_extent_buffer_operations(void);
 int btrfs_test_extent_io(void);
+int btrfs_test_inodes(void);
 int btrfs_init_test_fs(void);
 void btrfs_destroy_test_fs(void);
 struct inode *btrfs_new_test_inode(void);
@@ -49,6 +50,10 @@ static inline int btrfs_test_extent_io(void)
 {
return 0;
 }
+static inline int int btrfs_test_inodes(void)
+{
+   return 0;
+}
 #endif
 
 #endif
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
new file mode 100644
index 000..92416b3
--- /dev/null
+++ b/fs/btrfs/tests/inode-tests.c
@@ -0,0 +1,832 @@
+/*
+ * Copyright (C) 2013 Fusion IO.  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 v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will 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 

[GIT PULL] Btrfs

[Chris Mason]

Hi Linus,

We've got more bug fixes in my for-linus branch:

git://git.kernel.org/pub/scm/linux/kernel/git/mason/linux-btrfs.git for-linus

One of these fixes another corner of the compression oops from last
time.  Miao nailed down some problems with concurrent snapshot deletion
and drive balancing.

I kept out one of his patches for more testing, but these are all
stable.

Josef Bacik (2) commits (+5/-9):
Btrfs: limit delalloc pages outside of find_delalloc_range (+4/-8)
Btrfs: use right root when checking for hash collision (+1/-1)

Miao Xie (2) commits (+20/-12):
Btrfs: fix oops caused by the space balance and dead roots (+17/-7)
Btrfs: insert orphan roots into fs radix tree (+3/-5)

Total: (4) commits (+25/-21)

 fs/btrfs/disk-io.c|  9 +
 fs/btrfs/disk-io.h| 13 +++--
 fs/btrfs/extent_io.c  | 12 
 fs/btrfs/inode.c  |  2 +-
 fs/btrfs/relocation.c |  2 +-
 fs/btrfs/root-tree.c  |  8 +++-
 6 files changed, 25 insertions(+), 21 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