Date: Thu, 18 Jul 2013 21:52:15 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Andriy Gapon <avg@FreeBSD.org> Cc: freebsd-fs@FreeBSD.org, Adrian Chadd <adrian@FreeBSD.org> Subject: Re: Deadlock in nullfs/zfs somewhere Message-ID: <20130718185215.GE5991@kib.kiev.ua> In-Reply-To: <51E7F05A.5020609@FreeBSD.org> References: <CAJ-Vmomy3MrkSwJLQUGnDuD3EC3HzrudEghSDMeDwzVdaFNpLg@mail.gmail.com> <51DCFEDA.1090901@FreeBSD.org> <CAJ-VmokctCmV4%2By17uvqO9wXEyh0s%2BaXZ9nggvoAgP5%2BZHSgFA@mail.gmail.com> <51E59FD9.4020103@FreeBSD.org> <CAJ-VmokR8jJpdRc_kBJzhW4_R1pJnj3UPfsG5ANpq-kEGwCP9g@mail.gmail.com> <51E67F54.9080800@FreeBSD.org> <CAJ-Vmonk2HAzX38-mbL8hwxiUfL6JyJrMTq0dTBctW=P4dfyEQ@mail.gmail.com> <51E7B686.4090509@FreeBSD.org> <20130718112814.GA5991@kib.kiev.ua> <51E7F05A.5020609@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Thu, Jul 18, 2013 at 04:40:42PM +0300, Andriy Gapon wrote:
> on 18/07/2013 14:28 Konstantin Belousov said the following:
> > Well, I have no opinion. Making the fs suspended, in other words, preventing
> > writers from entering the filesystem code, is probably good. I do not
> > know zfs code to usefully comment on the approach.
>
> OK, fair.
>
> > Note that you must drain existing writers, i.e. call vfs_write_suspend(),
> > to set MNTK_SUSPEND.
>
> Here is my take on it, not tested at all.
>
> diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> index 0fc59cc..59c8cbd 100644
> --- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> +++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vfsops.c
> @@ -2263,8 +2263,12 @@ zfs_suspend_fs(zfsvfs_t *zfsvfs)
> {
> int error;
>
> - if ((error = zfsvfs_teardown(zfsvfs, B_FALSE)) != 0)
> + if ((error = vfs_write_suspend(zfsvfs->z_vfs)) != 0)
> return (error);
> + if ((error = zfsvfs_teardown(zfsvfs, B_FALSE)) != 0) {
> + vfs_write_resume(mp, 0);
> + return (error);
> + }
> dmu_objset_disown(zfsvfs->z_os, zfsvfs);
>
> return (0);
> @@ -2339,5 +2343,6 @@ bail:
> rrw_exit(&zfsvfs->z_teardown_lock, FTAG);
>
> + vfs_write_resume(mp, 0);
> if (err) {
> /*
> * Since we couldn't reopen zfsvfs::z_os, or
There is VFS method VFS_SUSP_CLEAN, called when the suspension is
lifted. UFS uses it to clean the back-queue of work which were
not performed during the suspend, mostly inactivate the postponed
inactive vnodes. ZFS probably does not need it, since it does
not check for MNTK_SUSPEND, but if it starts care, there is a place
to put the code.
On the other hand, I believe that your patch is notoriously incomplete,
because there should be a lot of threads which mutate ZFS mounts state
and which do not call vn_start_write() around the mutations. I.e.
all ZFS top-level code which calls into ZFS ops and which is not
coming from VFS.
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (FreeBSD)
iQIcBAEBAgAGBQJR6DlfAAoJEJDCuSvBvK1BMcAP/jw+dYeUw6RjJZCmJ7I6XrHa
aMYHMVjPuUH3h5jyE3avPZXd1OU67GkKmu+pQMF9qaMzPie6IkNK7FnK8Ey2Wy9D
1chQXv1ccaAvJTDk1VZbEWtctZ38V5CPm34ZbwGWk0wImjMU03C6D+fZRc9VzozM
hyxQAyDc89Nmmcxn6BnL4INJAAIASVB3QcRY2lO7/FKjAR/yxmy6R74/HvvRNDhk
QYvvSFJzNnB9wQByupYY69hbz18VQI3hyTMzh3xZkt9JcH2oCHanl8WkQUIxB4eJ
ZCBsKFZdV+rKLnXHfP4tlXmmXLTzFVJlf5u+vS1l4PhLWsV2IZceImFDRLaxq1OC
v2pF0DM2txWRODrsGY5Ie/DUwm7DoUghpu27rirkTcx84w3BWoD4/F5iEM1J+ZWY
MXyC7Gj4560v0lE4mDyw0ZKDVfOsmWH5dx4ElwFCk6Wvxk4/+Wg3qtnZlTsKMZkM
tJkw8lCj7xzg/FCg5ukXRnfuPixB3eiAbyEaQF1qR391YYypwfwSTfg3E2lYnSs0
BoijDzDaCj+8xlvl6CPY+/YKY6j1rTXFXcQQ5ayviOHVJjWo3kQAHukyWnZw/glx
fZR8rZkv13iAnXSxulJW4AZA3O0Ahy42IuWfVdL7g3cRFEzHPvkXkbi4h/l6J/Wf
uLiHucS8MyFMDh5GyDg/
=Bddh
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130718185215.GE5991>