Btrfs for 4.7, part 2

Hi,

please pull a few more patches that did not go to pull #1 for 4.7, minor cleanups and fixes. Thanks.

Quoted 4 lines · David Sterba

Hi,

please pull a few more patches that did not go to pull #1 for 4.7, minor cleanups and fixes. Thanks.

Thanks Dave! Trying to figure out why we're failing btrfs/011, but I don't see how it could be related to this bunch. I'll nail it down.

Quoted 6 lines · Chris Mason

Quoted 4 lines · David Sterba

Hi,

please pull a few more patches that did not go to pull #1 for 4.7, minor cleanups and fixes. Thanks.

Quoted 2 lines

Thanks Dave! Trying to figure out why we're failing btrfs/011, but I don't see how it could be related to this bunch. I'll nail it down.

011 passes here, there are some unrelated soft-failures (mismatching output with new progs). I'm now testing a branch without "btrfs: scrub:
Set bbio to NULL before calling btrfs_map_block", that seems to be the only likely offender.

Quoted 7 lines · David Sterba

Quoted 6 lines · Chris Mason

Quoted 4 lines · David Sterba

Hi,

please pull a few more patches that did not go to pull #1 for 4.7, minor cleanups and fixes. Thanks.

Quoted 7 lines

Quoted 2 lines

Thanks Dave! Trying to figure out why we're failing btrfs/011, but I don't see how it could be related to this bunch. I'll nail it down.

011 passes here, there are some unrelated soft-failures (mismatching output with new progs). I'm now testing a branch without "btrfs: scrub:
Set bbio to NULL before calling btrfs_map_block", that seems to be the only likely offender.

I'm getting errors from btrfs fi show -d, after the very last round of device replaces. A little extra debugging:

bytenr mismatch, want=4332716032, have=0
ERROR: cannot read chunk root
ERROR reading /dev/vdh
failed /dev/vdh

Which is cute because the very next command we run fscks /dev/vdh and succeeds.

So the page cache is stale and this isn't related to any of our patches.

Quoted 19 lines · Chris Mason

Quoted 7 lines · David Sterba

Quoted 6 lines · Chris Mason

Quoted 4 lines · David Sterba

Hi,

please pull a few more patches that did not go to pull #1 for 4.7, minor cleanups and fixes. Thanks.

Quoted 7 lines

Quoted 2 lines

Thanks Dave! Trying to figure out why we're failing btrfs/011, but I don't see how it could be related to this bunch. I'll nail it down.

011 passes here, there are some unrelated soft-failures (mismatching output with new progs). I'm now testing a branch without "btrfs: scrub:
Set bbio to NULL before calling btrfs_map_block", that seems to be the only likely offender.

I'm getting errors from btrfs fi show -d, after the very last round of device replaces. A little extra debugging:

Quoted 7 lines

bytenr mismatch, want=4332716032, have=0
ERROR: cannot read chunk root
ERROR reading /dev/vdh
failed /dev/vdh

Which is cute because the very next command we run fscks /dev/vdh and succeeds.

Quoted 1 line

So the page cache is stale and this isn't related to any of our patches.

close_ctree() calls into btrfs_close_devices(), which calls btrfs_close_one_device(), which uses:

call_rcu(&device->rcu, free_device);

close_ctree() also does an rcu_barrier() to make sure and wait for free_device() to finish.

But, free_device() just puts the work into schedule_work(), so we don't know for sure the blkdev_put is done when we exit.

It's been this way for a while, so its not holding up my pull request to Linus. But I'll fix it up.

-chris

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

Quoted 10 lines · Chris Mason

Quoted 10 lines

I'm getting errors from btrfs fi show -d, after the very last round of device replaces. A little extra debugging:

bytenr mismatch, want=4332716032, have=0
ERROR: cannot read chunk root
ERROR reading /dev/vdh
failed /dev/vdh

Which is cute because the very next command we run fscks /dev/vdh and succeeds.

Checked the code paths both btrfs fi show -d and btrfs check, both are calling flush during relative open_ctree in progs.

However the flush is called after we have read superblock. That means the read_superblock during 'show' cli (only) will read superblock without flush, and 'check' won't, because 011 calls 'check' after 'show'. But it still does not explain the above error, which is during open_ctree not at read superblock. Remains strange case as of now.

Also. I can't reproduce.

Quoted 7 lines

Quoted 1 line

So the page cache is stale and this isn't related to any of our patches.

close_ctree() calls into btrfs_close_devices(), which calls btrfs_close_one_device(), which uses:

call_rcu(&device->rcu, free_device);

Quoted 5 lines

close_ctree() also does an rcu_barrier() to make sure and wait for free_device() to finish.

But, free_device() just puts the work into schedule_work(), so we don't know for sure the blkdev_put is done when we exit.

Right, saw that before. Any idea why its like that ? Or if it should be fixed?

Quoted 17 lines · Anand Jain

Quoted 10 lines · Chris Mason

Quoted 10 lines

I'm getting errors from btrfs fi show -d, after the very last round of device replaces. A little extra debugging:

bytenr mismatch, want=4332716032, have=0
ERROR: cannot read chunk root
ERROR reading /dev/vdh
failed /dev/vdh

Which is cute because the very next command we run fscks /dev/vdh and succeeds.

Checked the code paths both btrfs fi show -d and btrfs check, both are calling flush during relative open_ctree in progs.

Quoted 6 lines

However the flush is called after we have read superblock. That means the read_superblock during 'show' cli (only) will read superblock without flush, and 'check' won't, because 011 calls 'check' after 'show'. But it still does not explain the above error, which is during open_ctree not at read superblock. Remains strange case as of now.

It's because we're just not done writing it out yet when btrfs fi show is run.
I think replace is special here.

Quoted 3 lines

Also. I can't reproduce.

I'm in a relatively new test rig using kvm, which probably explains why I haven't seen it before. You can probably make it easier by adding a sleep inside the actual __free_device() func.

Quoted 14 lines

Quoted 13 lines

Quoted 2 lines

So the page cache is stale and this isn't related to any of our patches.

close_ctree() calls into btrfs_close_devices(), which calls btrfs_close_one_device(), which uses:

call_rcu(&device->rcu, free_device);

close_ctree() also does an rcu_barrier() to make sure and wait for free_device() to finish.

But, free_device() just puts the work into schedule_work(), so we don't know for sure the blkdev_put is done when we exit.

Quoted 2 lines

Right, saw that before. Any idea why its like that ? Or if it should be fixed?

It's just trying to limit the work that is done from call_rcu, and it should
definitely be fixed. It might cause EBUSY or other problems. Probably easiest to add a counter or completion object that gets changed by the __free_device function.

-chris

Further to the previous commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

Since free_device() spinoff __free_device() the rcu_barrier() for

      call_rcu(&device->rcu, free_device);

didn't help.

This patch reverts changes by
bc178622d40d87e75abc131007342429c9b03351
and implement a method to wait on the __free_device() by using a new bdev_closing member in struct btrfs_device.

Signed-off-by Anand Jain <[email protected]>

[rework: bc178622d40d87e75abc131007342429c9b03351]

---
 fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++------
 fs/btrfs/volumes.h |  1 +
 2 files changed, 39 insertions(+), 6 deletions(-)

fs/btrfs/volumes.c +38 -6

index a4e8d48acd4b..404ce1daebb1 100644
@@ -27,6 +27,7 @@
 #include <linux/raid/pq.h>
 #include <linux/semaphore.h>
 #include <linux/uuid.h>
+#include <linux/delay.h>
 #include <asm/div64.h>
 #include "ctree.h"
 #include "extent_map.h"

@@ -254,6 +255,17 @@ static struct btrfs_device *__alloc_device(void)
 	return dev;
 }
 
+static int is_device_closing(struct list_head *head)
+{
+	struct btrfs_device *dev;
+
+	list_for_each_entry(dev, head, dev_list) {
+		if (dev->bdev_closing)
+			return 1;
+	}
+	return 0;
+}
+
 static noinline struct btrfs_device *__find_device(struct list_head *head,
 						   u64 devid, u8 *uuid)
 {

@@ -832,12 +844,22 @@ again:
 static void __free_device(struct work_struct *work)
 {
 	struct btrfs_device *device;
+	struct btrfs_device *new_device_addr;
 
 	device = container_of(work, struct btrfs_device, rcu_work);
 
 	if (device->bdev)
 		blkdev_put(device->bdev, device->mode);
 
+	/*
+	 * If we are coming here from btrfs_close_one_device()
+	 * then it allocates a new device structure for the same
+	 * devid, so find device again with the devid
+	 */
+	new_device_addr = __find_device(&device->fs_devices->devices,
+						device->devid, NULL);
+
+	new_device_addr->bdev_closing = 0;
 	rcu_string_free(device->name);
 	kfree(device);
 }

@@ -884,6 +906,12 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	list_replace_rcu(&device->dev_list, &new_device->dev_list);
 	new_device->fs_devices = device->fs_devices;
 
+	/*
+	 * So to wait for kworkers to finish all blkdev_puts,
+	 * so device is really free when umount is done.
+	 */
+	new_device->bdev_closing = 1;
+
 	call_rcu(&device->rcu, free_device);
 }
 

@@ -912,6 +940,7 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_fs_devices *seed_devices = NULL;
 	int ret;
+	int retry_cnt = 5;
 
 	mutex_lock(&uuid_mutex);
 	ret = __btrfs_close_devices(fs_devices);

@@ -927,12 +956,15 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 		__btrfs_close_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
-	/*
-	 * Wait for rcu kworkers under __btrfs_close_devices
-	 * to finish all blkdev_puts so device is really
-	 * free when umount is done.
-	 */
-	rcu_barrier();
+
+	while (is_device_closing(&fs_devices->devices) &&
+						--retry_cnt) {
+		mdelay(1000); //1 sec
+	}
+
+	if (!(retry_cnt > 0))
+		printk(KERN_WARNING "BTRFS: %pU bdev_put didn't complete, giving up\n",
+			fs_devices->fsid);
 	return ret;
 }
 

fs/btrfs/volumes.h +1 -0

index 0ac90f8d85bd..945e49f5e17d 100644
@@ -150,6 +150,7 @@ struct btrfs_device {
 	/* Counter to record the change of device stats */
 	atomic_t dev_stats_ccnt;
 	atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
+	int bdev_closing;
 };
 
 /*

Signature · 1 line

2.7.0

Quoted 127 lines · Anand Jain

Further to the previous commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

Since free_device() spinoff __free_device() the rcu_barrier() for

      call_rcu(&device->rcu, free_device);

didn't help.

This patch reverts changes by
bc178622d40d87e75abc131007342429c9b03351
and implement a method to wait on the __free_device() by using a new bdev_closing member in struct btrfs_device.

Signed-off-by Anand Jain <[email protected]>

[rework: bc178622d40d87e75abc131007342429c9b03351]

---
 fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++------
 fs/btrfs/volumes.h |  1 +
 2 files changed, 39 insertions(+), 6 deletions(-)

fs/btrfs/volumes.c +38 -6

index a4e8d48acd4b..404ce1daebb1 100644
@@ -27,6 +27,7 @@
 #include <linux/raid/pq.h>
 #include <linux/semaphore.h>
 #include <linux/uuid.h>
+#include <linux/delay.h>
 #include <asm/div64.h>
 #include "ctree.h"
 #include "extent_map.h"

@@ -254,6 +255,17 @@ static struct btrfs_device *__alloc_device(void)
 	return dev;
 }
 
+static int is_device_closing(struct list_head *head)
+{
+	struct btrfs_device *dev;
+
+	list_for_each_entry(dev, head, dev_list) {
+		if (dev->bdev_closing)
+			return 1;
+	}
+	return 0;
+}
+
 static noinline struct btrfs_device *__find_device(struct list_head *head,
 						   u64 devid, u8 *uuid)
 {

@@ -832,12 +844,22 @@ again:
 static void __free_device(struct work_struct *work)
 {
 	struct btrfs_device *device;
+	struct btrfs_device *new_device_addr;
 
 	device = container_of(work, struct btrfs_device, rcu_work);
 
 	if (device->bdev)
 		blkdev_put(device->bdev, device->mode);
 
+	/*
+	 * If we are coming here from btrfs_close_one_device()
+	 * then it allocates a new device structure for the same
+	 * devid, so find device again with the devid
+	 */
+	new_device_addr = __find_device(&device->fs_devices->devices,
+						device->devid, NULL);
+
+	new_device_addr->bdev_closing = 0;
 	rcu_string_free(device->name);
 	kfree(device);
 }

@@ -884,6 +906,12 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	list_replace_rcu(&device->dev_list, &new_device->dev_list);
 	new_device->fs_devices = device->fs_devices;
 
+	/*
+	 * So to wait for kworkers to finish all blkdev_puts,
+	 * so device is really free when umount is done.
+	 */
+	new_device->bdev_closing = 1;
+
 	call_rcu(&device->rcu, free_device);
 }
 

@@ -912,6 +940,7 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 {
 	struct btrfs_fs_devices *seed_devices = NULL;
 	int ret;
+	int retry_cnt = 5;
 
 	mutex_lock(&uuid_mutex);
 	ret = __btrfs_close_devices(fs_devices);

@@ -927,12 +956,15 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 		__btrfs_close_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
-	/*
-	 * Wait for rcu kworkers under __btrfs_close_devices
-	 * to finish all blkdev_puts so device is really
-	 * free when umount is done.
-	 */
-	rcu_barrier();
+
+	while (is_device_closing(&fs_devices->devices) &&
+						--retry_cnt) {
+		mdelay(1000); //1 sec
+	}
+
+	if (!(retry_cnt > 0))
+		printk(KERN_WARNING "BTRFS: %pU bdev_put didn't complete, giving up\n",
+			fs_devices->fsid);
 	return ret;
 }
 

fs/btrfs/volumes.h +1 -0

index 0ac90f8d85bd..945e49f5e17d 100644
@@ -150,6 +150,7 @@ struct btrfs_device {
 	/* Counter to record the change of device stats */
 	atomic_t dev_stats_ccnt;
 	atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
+	int bdev_closing;
 };
 

Quoted 3 lines

fs/btrfs/volumes.h +0 -1

 /*
--

2.7.0

I gave this a try and somehow it seems to make unmounting worse:
it now always takes ~5s (max retry time) and I see the warning every time. Without the patch unmounting a single volume (disk) is much faster (1-2s), without problems.

Any ideas?

cheers,
Holger

From: Anand Jain <[email protected]>

Further to the commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

This patch implements a method to time wait on the __free_device() which actually does the bdev put. This is needed as the user space running 'btrfs fi show -d' immediately after the replace and unmount, is still reading older information from the device.

mail-archive.com/[email protected]/msg54188.html

Signed-off-by Anand Jain <[email protected]>

[updates: bc178622d40d87e75abc131007342429c9b03351]

---
v2: Also to make sure bdev_closing is set it needs rcu_barrier(),
restored rcu_barrier().

From: Anand Jain <[email protected]>

Further to the commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

This patch implements a method to time wait on the __free_device() which actually does the bdev put. This is needed as the user space running 'btrfs fi show -d' immediately after the replace and unmount, is still reading older information from the device.

Quoted 10 lines · Anand Jain

From: Anand Jain <[email protected]>

Further to the commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

This patch implements a method to time wait on the __free_device() which actually does the bdev put. This is needed as the user space running 'btrfs fi show -d' immediately after the replace and unmount, is still reading older information from the device.

Thanks for working on this Anand. Since it looks like blkdev_put can deadlock against us, can we please switch to making sure we fully flush the outstanding IO? It's probably enough to do a sync_blockdev() call before we allow the unmount to finish, but we can toss in an invalidate_bdev for good measure.

Then we can get rid of the mdelay loop completely, which seems pretty error prone to me.

Thanks!

-chris

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

Thanks for the review Chris.

Quoted 12 lines · Chris Mason

Quoted 10 lines · Anand Jain

From: Anand Jain <[email protected]>

Further to the commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

This patch implements a method to time wait on the __free_device() which actually does the bdev put. This is needed as the user space running 'btrfs fi show -d' immediately after the replace and unmount, is still reading older information from the device.

Quoted 5 lines

Thanks for working on this Anand. Since it looks like blkdev_put can deadlock against us, can we please switch to making sure we fully flush the outstanding IO? It's probably enough to do a sync_blockdev() call before we allow the unmount to finish, but we can toss in an invalidate_bdev for good measure.

------------

# git diff

fs/btrfs/volumes.c +5 -0

index 604daf315669..e0ad29d6fe9a 100644
@@ -870,6 +870,11 @@ static void btrfs_close_one_device(struct 
 btrfs_device *device)
        if (device->missing)
                fs_devices->missing_devices--;
 
+       if (device->bdev && device->writeable) {
+               sync_blockdev(device->bdev);
+               invalidate_bdev(device->bdev);
+       }
+
        new_device = btrfs_alloc_device(NULL, &device->devid,
                                        device->uuid);

        BUG_ON(IS_ERR(new_device)); /* -ENOMEM */

-----------

However, theoretically still there might be a problem - at the end of unmount, if the device exclusive open is not actually closed, then there might be a race with another program which is trying to open the device in exclusive mode. Like for eg:
unmount /btrfs; fsck /dev/X
and here fsck might fail to open the device if it wins the race.

Quoted 46 lines · Anand Jain

Thanks for the review Chris.

Quoted 12 lines · Chris Mason

Quoted 10 lines · Anand Jain

From: Anand Jain <[email protected]>

Further to the commit
bc178622d40d87e75abc131007342429c9b03351
btrfs: use rcu_barrier() to wait for bdev puts at unmount

This patch implements a method to time wait on the __free_device() which actually does the bdev put. This is needed as the user space running 'btrfs fi show -d' immediately after the replace and unmount, is still reading older information from the device.

Quoted 5 lines

Thanks for working on this Anand. Since it looks like blkdev_put can deadlock against us, can we please switch to making sure we fully flush the outstanding IO? It's probably enough to do a sync_blockdev() call before we allow the unmount to finish, but we can toss in an invalidate_bdev for good measure.

------------

# git diff

fs/btrfs/volumes.c +5 -0

index 604daf315669..e0ad29d6fe9a 100644
@@ -870,6 +870,11 @@ static void btrfs_close_one_device(struct
 btrfs_device *device)
        if (device->missing)
                fs_devices->missing_devices--;
 
+       if (device->bdev && device->writeable) {
+               sync_blockdev(device->bdev);
+               invalidate_bdev(device->bdev);
+       }
+
        new_device = btrfs_alloc_device(NULL, &device->devid,
                                        device->uuid);

        BUG_ON(IS_ERR(new_device)); /* -ENOMEM */

-----------

Quoted 6 lines

However, theoretically still there might be a problem - at the end of unmount, if the device exclusive open is not actually closed, then there might be a race with another program which is trying to open the device in exclusive mode. Like for eg:
unmount /btrfs; fsck /dev/X
and here fsck might fail to open the device if it wins the race.

This true, but at least we know he'll have up to date buffers if he does manage to open the device.

With the generic code, the blkdev_put happens after the super is gone, so I'm not sure we can completely fix this from inside our callback.

-chris

An inconsistent behavior due to stale reads from the disk was reported

mail-archive.com/[email protected]/msg54188.html

This patch will make sure devices are synced before return in the unmount thread.

Signed-off-by Anand Jain <[email protected]>

---
v2: Also to make sure bdev_closing is set it needs rcu_barrier(),
restored rcu_barrier().
v3: Removed the complicated timed-wait on blkdev_put code,
but make synced and invalidated before umount is returned. So sorry that I have to change the title of this patch as well (was: btrfs: wait for bdev put).

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

fs/btrfs/volumes.c +0 -0

index 604daf315669..f741ade130a4 100644
@@ -870,6 +870,11 @@ static void btrfs_close_one_device(struct btrfs_device *device)
 	if (device->missing)

An inconsistent behavior due to stale reads from the disk was reported

mail-archive.com/[email protected]/msg54188.html

This patch will make sure devices are synced before return in the unmount thread.

Signed-off-by Anand Jain <[email protected]>