diff mbox

use rcu_barrier() to wait for bdev puts at unmount

Message ID 513AC735.1090500@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Sandeen March 9, 2013, 5:23 a.m. UTC
Doing this would reliably fail with -EBUSY for me:

# mount /dev/sdb2 /mnt/scratch; umount /mnt/scratch; mkfs.btrfs -f /dev/sdb2
...
unable to open /dev/sdb2: Device or resource busy

because mkfs.btrfs tries to open the device O_EXCL, and somebody still has it.

Using systemtap to track bdev gets & puts shows a kworker thread doing a
blkdev put after mkfs attempts a get; this is left over from the unmount.

Adding an rcu_barrier() to btrfs_close_devices() causes unmount to wait
until all blkdev_put()s are done, and the device is truly free once
unmount completes.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

p.s. I debated putting it into close_ctree(); I don't know if there' anything
else to wait for.  Thoughts?


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

Comments

Chris Mason March 9, 2013, 12:27 p.m. UTC | #1
On Fri, Mar 08, 2013 at 10:23:01PM -0700, Eric Sandeen wrote:
> Doing this would reliably fail with -EBUSY for me:
> 
> # mount /dev/sdb2 /mnt/scratch; umount /mnt/scratch; mkfs.btrfs -f /dev/sdb2
> ...
> unable to open /dev/sdb2: Device or resource busy
> 
> because mkfs.btrfs tries to open the device O_EXCL, and somebody still has it.
> 
> Using systemtap to track bdev gets & puts shows a kworker thread doing a
> blkdev put after mkfs attempts a get; this is left over from the unmount.
> 
> Adding an rcu_barrier() to btrfs_close_devices() causes unmount to wait
> until all blkdev_put()s are done, and the device is truly free once
> unmount completes.

Thanks for tracking this down Eric.  Is this kworker triggered by btrfs
or is this something we should be doing for the other filesystems too?

I'd move it down to close_ctree, but I don't really have a good reason.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen March 9, 2013, 2:17 p.m. UTC | #2
On 3/9/13 6:27 AM, Chris Mason wrote:
> On Fri, Mar 08, 2013 at 10:23:01PM -0700, Eric Sandeen wrote:
>> Doing this would reliably fail with -EBUSY for me:
>>
>> # mount /dev/sdb2 /mnt/scratch; umount /mnt/scratch; mkfs.btrfs -f /dev/sdb2
>> ...
>> unable to open /dev/sdb2: Device or resource busy
>>
>> because mkfs.btrfs tries to open the device O_EXCL, and somebody still has it.
>>
>> Using systemtap to track bdev gets & puts shows a kworker thread doing a
>> blkdev put after mkfs attempts a get; this is left over from the unmount.
>>
>> Adding an rcu_barrier() to btrfs_close_devices() causes unmount to wait
>> until all blkdev_put()s are done, and the device is truly free once
>> unmount completes.
> 
> Thanks for tracking this down Eric.  

Sure thing, sorry it took so long.

> Is this kworker triggered by btrfs
> or is this something we should be doing for the other filesystems too?

It's all btrfs ;)

btrfs_close_devices
	__btrfs_close_devices
		call_rcu(&device->rcu, free_device);
			free_device
				INIT_WORK(&device->rcu_work, __free_device);
				schedule_work(&device->rcu_work);


The behavior came from:

commit 1f78160ce1b1b8e657e2248118c4d91f881763f0
Author: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Date:   Wed Apr 20 10:09:16 2011 +0000

    Btrfs: using rcu lock in the reader side of devices list

Anyway, I can send V2 in close_ctree if you like. Thinking about it more
though, btrfs_close_devices is closer to the action, so now I think
I'd leave it there. :)

I probably should have put a comment in to say what the heck it's for,
too.  Feel free to fix on merge or I can send another patch.

Thanks,
-Eric


> I'd move it down to close_ctree, but I don't really have a good reason.
> 
> -chris
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Mason March 9, 2013, 3:03 p.m. UTC | #3
On Sat, Mar 09, 2013 at 07:17:04AM -0700, Eric Sandeen wrote:
> On 3/9/13 6:27 AM, Chris Mason wrote:
> > On Fri, Mar 08, 2013 at 10:23:01PM -0700, Eric Sandeen wrote:
> >> Doing this would reliably fail with -EBUSY for me:
> >>
> >> # mount /dev/sdb2 /mnt/scratch; umount /mnt/scratch; mkfs.btrfs -f /dev/sdb2
> >> ...
> >> unable to open /dev/sdb2: Device or resource busy
> >>
> >> because mkfs.btrfs tries to open the device O_EXCL, and somebody still has it.
> >>
> >> Using systemtap to track bdev gets & puts shows a kworker thread doing a
> >> blkdev put after mkfs attempts a get; this is left over from the unmount.
> >>
> >> Adding an rcu_barrier() to btrfs_close_devices() causes unmount to wait
> >> until all blkdev_put()s are done, and the device is truly free once
> >> unmount completes.
> > 
> > Thanks for tracking this down Eric.  
> 
> Sure thing, sorry it took so long.
> 
> > Is this kworker triggered by btrfs
> > or is this something we should be doing for the other filesystems too?
> 
> It's all btrfs ;)
> 
> btrfs_close_devices
> 	__btrfs_close_devices
> 		call_rcu(&device->rcu, free_device);
> 			free_device
> 				INIT_WORK(&device->rcu_work, __free_device);
> 				schedule_work(&device->rcu_work);
> 

Great, that makes a ton more sense.  I'm a little confused on why we're
seeing it so much more now than in the past.

> 
> The behavior came from:
> 
> commit 1f78160ce1b1b8e657e2248118c4d91f881763f0
> Author: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Date:   Wed Apr 20 10:09:16 2011 +0000
> 
>     Btrfs: using rcu lock in the reader side of devices list
> 
> Anyway, I can send V2 in close_ctree if you like. Thinking about it more
> though, btrfs_close_devices is closer to the action, so now I think
> I'd leave it there. :)
> 
> I probably should have put a comment in to say what the heck it's for,
> too.  Feel free to fix on merge or I can send another patch.

Ok, please add the comment and a cc to stable.  Thanks again.

-chris

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

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5cbb7f4..258316a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -680,6 +680,7 @@  int btrfs_close_devices(struct btrfs_fs_devices *fs_devices)
 		__btrfs_close_devices(fs_devices);
 		free_fs_devices(fs_devices);
 	}
+	rcu_barrier();
 	return ret;
 }