Need to force sync(2) before umounting UFS1 filesystems?

[Attilio Rao]

2011/9/29 Kirk McKusick <mckusick at mckusick.com>:
>> Date: Thu, 29 Sep 2011 17:40:43 +0200
>> Subject: Re: Need to force sync(2) before umounting UFS1 filesystems?
>> From: Attilio Rao <attilio at freebsd.org>
>> To: Kirk McKusick <mckusick at mckusick.com>
>> Cc: Garrett Cooper <yanegomi at gmail.com>, freebsd-fs at freebsd.org,
>>         Xin LI <delphij at freebsd.org>
>>
>> 2011/9/29 Kirk McKusick <mckusick at mckusick.com>:
>>
>> > Thanks for providing a bit more of the history on this codepath.
>> >
>> > Since 9-stable has now been branched, I believe that the best path
>> > forward is to check this change into head and let it sit there for
>> > several months so that we can get some experience with it. If it
>> > causes folks problems we can back it out. If it does not cause
>> > problems, then we can MFC it to 9-stable.
>> >
>> > Does this seem like a reasonable approach?
>>
>> In general yes, but I'd like to understand why unmount should fail so
>> much with SU... do we do extended period with vfs_busy()'ed
>> filesystem?
>>
>> I need more context here, likely I'd need to look into the PRs too
>> before to give an informative answer.
>>
>> Attilio
>
> I am definitely not in a rush on this, so by all means take some time
> to look it over. The EBUSY unmount has been in its current state
> for several years, so I am fine with taking a few weeks to sort out
> the correct solution. Indeed, I am glad that Garrett has volunteered
> to do some more serious testing.
>
> If this general approach is not correct, I can put a hook in for just
> UFS so that it can have its historic behavior. As you have noted, the
> SU code has a lot of activity that gets done under the protection of
> vfs_busy. So it may be the only filesystem for which draining the
> vfs_busy lock during unmount is needed.

Honestly, my first thought was exactly that -- an option that was
forcing the unmount sleep if SU is compiled in the kernel.

When you mention 'historical behaviour' you mean the behaviour UFS had
even prior the introduction of the 'complete' VFS layer or it was the
behaviour unmount(2) was expected to implemented since the beginning?

My guess is that recent SU improvement by you and Jeff may have lead
to higher vfs_busy() contention, thus making this behaviour just more
visible.

BTW, I'm afraid the forced unmount case may have a possible deadlock.

thread1 is doing whatever codepath it wants and thread2 is doing
unmount (forced right now):

thread1::vfs_busy()
thread2::lock coveredvnode
thread1::contests coveredvnode
thread2::sleep because of thread1 vfs_busy

I think this deadlock was actually possible even with the old code, it
was just a LOR between a vnode lock and mount lock.

I'm not sure if there was any invariant I discussed with Kostik in the
past, preventing one way or another I'm forgetting about, but it seems
a possible deadlock to me.

If you see this issue I'll make a patch for it.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein