Re: [PATCH] Fixing soft NFS umount -f, round 5

[Chuck Silvers]

On Fri, Jul 10, 2015 at 03:43:54AM +0000, Emmanuel Dreyfus wrote:
> On Wed, Jul 08, 2015 at 06:07:11PM +0000, Emmanuel Dreyfus wrote:
> > http://ftp.espci.fr/shadow/manu/umount_f4.patch
> >
> > With default values, timeout = 3 and retrans = 10, we now wait
> > for ages or the unmount to completes. In the umount -f case
> > it does not makes sense to wait for too long.
>
> Here is an improved version that still use your proposed timeouts:
> http://ftp.espci.fr/shadow/manu/umount_f5.patch

the new variable "before_ts" should be time_t rather than int.

it looks like "nmp->nm_iflag" is supposed to be protected by "nmp->nm_lock",
you ought to use hold that lock while modifying nm_iflag in nfs_unmount().

otherwise the latest diff looks ok to me.


> I introduced a NFS mount private flag NFSMNT_DISMNTFORCE that informs
> the NFS subsystem that we want to forcibly unmount the filesystem,
> which can be done at the expense of cutting timeout corners.
>
> An important point is that with the flag set, we do not attempt any
> NFS server reconnect. It makes sense since dounmount() calls
> VFS_SYNC() before VFS_UNMOUNT(). NFSMNT_DISMNTFORCE being set in
> VFS_UNMOUNT() / nfs_unmount(), we already made reconnection attempts
> withinin VFS_SYNC(), hence thereis benefit for doint it again.
>
> Now with that change, in the umount -f while NFS server is gone case,
> we spend 20s in VFS_SYNC(), and VFS_UNMOUNT() returns in less than a
> second.
>
> Now I think it would be nice to also cut coners in VFS_SYNC() when
> the force flag is used, but that touches filesystem-indpendent code,
> in dounmount():
> if ((mp->mnt_flag & MNT_RDONLY) == 0) {
> error = VFS_SYNC(mp, MNT_WAIT, l->l_cred);
> }
> if (error == 0 || (flags & MNT_FORCE)) {
> error = VFS_UNMOUNT(mp, flags);
> }
>
> The first VFS_SYNC() is making us wait even if MNT_FORCE is set. This could
> be solved by adding a IMNT_UMOUNTFORCE to struct mount's mnt_iflag, just
> like I did for NFS in this patch. The flag would instruct underlying
> filesystem that force unmount is required and that fast return is expected.

I haven't kept up with the VFS-level locking changes in the last few years
so I'll let someone else comment about all that.

in particular, I don't know what (if anything) prevents vnodes from becoming
dirty again in between the VFS_SYNC() and the VFS_UNMOUNT(). every file system
driver appears to flush everything again (via vflush()) in its *_unmount()
method, so I don't know what benefit the VFS_SYNC() in dounmount() is providing.

-Chuck


> Opinions?
>
> --
> Emmanuel Dreyfus
> ma...@netbsd.org

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-...@muc.de

[Emmanuel Dreyfus]

Anyone has an idea why we need this VFS_SYNC() before VFS_UNMOUNT() in
dounmount()? As noted by Chuck below, this seems to just duplicate
functionality. And it is at the expense of disturbing force unmount
since the VFS_SYNC ignoes the MNT_FORCE flags and can make us wait for a
while.

Chuck Silvers <ch...@chuq.com> wrote:

> > Now I think it would be nice to also cut coners in VFS_SYNC() when
> > the force flag is used, but that touches filesystem-indpendent code,
> > in dounmount():
> > if ((mp->mnt_flag & MNT_RDONLY) == 0) {
> > error = VFS_SYNC(mp, MNT_WAIT, l->l_cred);
> > }
> > if (error == 0 || (flags & MNT_FORCE)) {
> > error = VFS_UNMOUNT(mp, flags);
> > }
> >
> > The first VFS_SYNC() is making us wait even if MNT_FORCE is set. This could
> > be solved by adding a IMNT_UMOUNTFORCE to struct mount's mnt_iflag, just
> > like I did for NFS in this patch. The flag would instruct underlying
> > filesystem that force unmount is required and that fast return is expected.
>
> I haven't kept up with the VFS-level locking changes in the last few years
> so I'll let someone else comment about all that.
>
> in particular, I don't know what (if anything) prevents vnodes from
> becoming dirty again in between the VFS_SYNC() and the VFS_UNMOUNT().
> every file system driver appears to flush everything again (via vflush())
> in its *_unmount() method, so I don't know what benefit the VFS_SYNC() in
> dounmount() is providing.

--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz

[Emmanuel Dreyfus]

Emmanuel Dreyfus <ma...@netbsd.org> wrote:

> Anyone has an idea why we need this VFS_SYNC() before VFS_UNMOUNT() in
> dounmount()? As noted by Chuck below, this seems to just duplicate
> functionality. And it is at the expense of disturbing force unmount
> since the VFS_SYNC ignoes the MNT_FORCE flags and can make us wait for a
> while.

No complain on this, anyone? I tested removing this VFS_SYNC(), and NFS
force umount now respond faster without other processes hanging in
tstile until it is done.

Shall I commit it?

[David Holland]

On Sun, Jul 12, 2015 at 06:54:21PM -0700, Chuck Silvers wrote:
> > Now I think it would be nice to also cut coners in VFS_SYNC() when
> > the force flag is used, but that touches filesystem-indpendent code,
> > in dounmount():
> > if ((mp->mnt_flag & MNT_RDONLY) == 0) {
> > error = VFS_SYNC(mp, MNT_WAIT, l->l_cred);
> > }
> > if (error == 0 || (flags & MNT_FORCE)) {
> > error = VFS_UNMOUNT(mp, flags);
> > }
> >
> > The first VFS_SYNC() is making us wait even if MNT_FORCE is
> > set. This could be solved by adding a IMNT_UMOUNTFORCE to struct
> > mount's mnt_iflag, just like I did for NFS in this patch. The
> > flag would instruct underlying filesystem that force unmount is
> > required and that fast return is expected.

Can this please be an argument to sync instead of a global state flag?
If nfs needs to make it an internal global state flag that's one
thing, but in general the proliferation of such flags is bad.

> I haven't kept up with the VFS-level locking changes in the last
> few years so I'll let someone else comment about all that.
>
> in particular, I don't know what (if anything) prevents vnodes from
> becoming dirty again in between the VFS_SYNC() and the
> VFS_UNMOUNT(). every file system driver appears to flush
> everything again (via vflush()) in its *_unmount() method, so I
> don't know what benefit the VFS_SYNC() in dounmount() is providing.

I don't know offhand, but in another environment I've been working in
recently there's such a sync call and it works under the following
rules:
- the vfs code locks the mount point of the fs before calling sync
and unmount, so if the fs is not busy nothing can enter it;
- the first thing the fs does in unmount is check if it's busy, and
if so unmount fails;
- if the fs is not busy there are no outstanding vnode references,
so no more can be generated because of the vfs-level locking;
- therefore unmount can proceed knowing both that the fs is already
synced and that nothing can make it dirty again.

This works reasonably well; however, I don't know without wading in
how much relevance it might have to the way things do or might be made
to work in netbsd.

Offhand it seems to me that since the vnode cache is now fully
vfs-level (thanks to hannken@, who's been doing all my work for me for
like the last two years) that the vfs code now ought to be able to
handle the busy check itself too. But I have no immediate idea how
feasible that really is.

--
David A. Holland
dhol...@netbsd.org

[Emmanuel Dreyfus]

David Holland <dholla...@netbsd.org> wrote:

> I don't know offhand, but in another environment I've been working in
> recently there's such a sync call and it works under the following
> rules:

And there is no diference between force and regular unmount?

--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
ma...@netbsd.org

[Thor Lancelot Simon]

On Tue, Jul 14, 2015 at 06:58:27PM +0200, Emmanuel Dreyfus wrote:
> Emmanuel Dreyfus <ma...@netbsd.org> wrote:
>
> > Anyone has an idea why we need this VFS_SYNC() before VFS_UNMOUNT() in
> > dounmount()? As noted by Chuck below, this seems to just duplicate
> > functionality. And it is at the expense of disturbing force unmount
> > since the VFS_SYNC ignoes the MNT_FORCE flags and can make us wait for a
> > while.
>
> No complain on this, anyone? I tested removing this VFS_SYNC(), and NFS
> force umount now respond faster without other processes hanging in
> tstile until it is done.

I objected the last time you brought it up.

> Shall I commit it?

Not unless it's conditionalized so it only happens if the umount is forced.

Thor

[Emmanuel Dreyfus]

Thor Lancelot Simon <t...@panix.com> wrote:

> I objected the last time you brought it up.

You refer to this?

> It's not required to, but I am glad it does. How else can you actually
> know all the buffers hit the disk before you hit the power switch?

As noted by Chuck in the meantime, VFS_UNMOUNT() also flushes data (with
the knowledge of wether this is a forced unmount or not), and nothing
seems to prevent new dirty data to be added between the VFS_SYNC() and
VFS_UNMOUNT().

This suggests the VFS_SYNC() just duplicates the flush operation, adding
delay because it does not know about force unmount, and without any real
benefit.

--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
ma...@netbsd.org

[matthew green]

Emmanuel Dreyfus writes:
> Emmanuel Dreyfus <ma...@netbsd.org> wrote:
>
> > Anyone has an idea why we need this VFS_SYNC() before VFS_UNMOUNT() in
> > dounmount()? As noted by Chuck below, this seems to just duplicate
> > functionality. And it is at the expense of disturbing force unmount
> > since the VFS_SYNC ignoes the MNT_FORCE flags and can make us wait for a
> > while.
>
> No complain on this, anyone? I tested removing this VFS_SYNC(), and NFS
> force umount now respond faster without other processes hanging in
> tstile until it is done.
>
> Shall I commit it?

please wait at least a week, if not 2 or 3, before commiting this
potentially dangerous change, for other comments.


.mrg.

[Emmanuel Dreyfus]

matthew green <m...@eterna.com.au> wrote:

> please wait at least a week, if not 2 or 3, before commiting this
> potentially dangerous change, for other comments.

Well, one week is fine, more is only reasonable if some discussion is
ongoing...

--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
ma...@netbsd.org