-------- Original Message --------
Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for
processing pending changes" related commits
From: Qu Wenruo <quwen...@cn.fujitsu.com>
To: dste...@suse.cz, linux-btrfs@vger.kernel.org, miao...@huawei.com
Date: 2015年01月26日 08:37
-------- Original Message --------
Subject: Re: [PATCH RFC v3 1/5] Revert "btrfs: add support for
processing pending changes" related commits
From: David Sterba <dste...@suse.cz>
To: Qu Wenruo <quwen...@cn.fujitsu.com>
Date: 2015年01月23日 22:57
On Fri, Jan 23, 2015 at 05:31:41PM +0800, Qu Wenruo wrote:
For mount option change, later patches will introduce copy-n-update
method and rwsem protects to keep mount options consistent during
transaction.
That's a better approach, for the mount options.
I'm glad that you like this method.
Although the description in this patch is outdated, it is now
per-transaction mount option.
Sorry for the confusion.
For sysfs interface to change label/features, it will keep the same
behavior as 'btrfs pro set', so pending changes are also not needed.
This still leaves the transaction commit inside the syfs handler, that
was one of the points not to do that.
The callstack looks safe from, eg. the label handler:
[169148.523158] WARNING: CPU: 1 PID: 2044 at fs/btrfs/sysfs.c:394
btrfs_label_store+0x135/0x190 [btrfs]()
[169148.533925] Modules linked in: btrfs dm_flakey rpcsec_gss_krb5
loop [last unloaded: btrfs]
[169148.536950] CPU: 1 PID: 2044 Comm: bash Tainted: G W
3.19.0-rc5-default+ #211
[169148.536952] Hardware name: Intel Corporation Santa Rosa
platform/Matanzas, BIOS TSRSCRB1.86C.0047.B00.0610170821 10/17/06
[169148.536954] 000000000000018a ffff88007a753dc8 ffffffff81a9898b
000000000000018a
[169148.536963] 0000000000000000 ffff88007a753e08 ffffffff81077f65
ffff880077fb0100
[169148.536972] ffff880075dc0000 ffff880077fbff00 0000000000000009
ffff880075dc06d0
[169148.536980] Call Trace:
[169148.536983] [<ffffffff81a9898b>] dump_stack+0x4f/0x6c
[169148.536991] [<ffffffff81077f65>] warn_slowpath_common+0x95/0xe0
[169148.537000] [<ffffffff81077fca>] warn_slowpath_null+0x1a/0x20
[169148.537005] [<ffffffffa0052b65>] btrfs_label_store+0x135/0x190
[btrfs]
[169148.537030] [<ffffffff813ed8b7>] kobj_attr_store+0x17/0x20
[169148.537037] [<ffffffff812147ff>] sysfs_kf_write+0x4f/0x70
[169148.537044] [<ffffffff81213cc8>] kernfs_fop_write+0x128/0x180
[169148.537051] [<ffffffff8119f404>] vfs_write+0xd4/0x1d0
[169148.537059] [<ffffffff8119f7b9>] SyS_write+0x59/0xd0
[169148.537070] [<ffffffff81a9f9d2>] system_call_fastpath+0x12/0x17
Lockep shows these locks held:
[169148.537296] 4 locks held by bash/2044:
[169148.537309] #0: (sb_writers#5){.+.+.+}, at:
[<ffffffff8119f4e0>] vfs_write+0x1b0/0x1d0
[169148.537319] #1: (&of->mutex){+.+.+.}, at: [<ffffffff81213c2e>]
kernfs_fop_write+0x8e/0x180
[169148.537330] #2: (s_active#214){.+.+.+}, at:
[<ffffffff81213c36>] kernfs_fop_write+0x96/0x180
[169148.537342] #3: (tasklist_lock){.+.+..}, at:
[<ffffffff810b9ed4>] debug_show_all_locks+0x44/0x1e0
#3 is from lockdep
#2 is not really a lock, annotated vfs atomic counter
#0 is annotated atomic, the freezing barrier
#1 is a kernfs mutex that, afaics it's per file, but I don't like to see
the lock dependency here. That's a lock we can see now, but it's outside
of btrfs or the vfs. It's a matter of precaution.
Thanks for pointing out the problem.
It makes sense to delay it.
But we have btrfs-workqueue, why not put it to "worker" workqueue?
If using this method, we can just wrap btrfs_ioctl_set_fslabel() and
queue it to fs_info->workers.
This can avoid the the lockdep problem, but the behavior is still
inconsistent with the synchronized
ioctl method.
Although not perfect, it should be good enough and still clean enough.
Wait a second, #1 is a mutex, so I didn't quite understand the problem.
Just because it is not btrfs/vfs mutex so we want to avoid it?
It seems not convincing enough for me...
For readonly/freeze check, I prefer extra vfsmount from sb->s_mounts and
use mnt_want_write() (handle ro)
and transaction (handle freeze).
So IMHO it just needs some small tweaks on the original implementation.
Thanks,
Qu
What do you think about such method?
Thanks,
Qu
--
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
