Re: [PATCH 1/3] nfsd: use __fput_sync() to avoid delayed closing of files.

[NeilBrown]

On Tue, 12 Dec 2023, Al Viro wrote:
> On Mon, Dec 11, 2023 at 09:47:35AM +1100, NeilBrown wrote:
> 
> > Similarly would could wrap __fput_sync() is a more friendly name, but
> > that would be better if we actually renamed the function.
> > 
> >   void fput_now(struct file *f)
> >   {
> >       __fput_sync(f);
> >   }
> 
> It is unfriendly *precisely* because it should not be used without
> a very good reason.
> 
> It may be the last opened file keeping a lazy-umounted mount alive.
> It may be taking pretty much any locks, or eating a lot of stack
> space.

Previously you've suggested problems with ->release blocking.
Now you refer to lazy-umount, which is what the comment above
__fput_sync() mentions.

"pretty much an locks" seems like hyperbole.  I don't see it taking
nfsd_mutex or nlmsvc_mutex.  Maybe you mean any filesystem lock?

My understanding is that the advent of vmalloc allocated stacks means
that kernel stack space is not an important consideration.

It would really help if we could have clear documented explanation of
what problems can occur.  Maybe an example of contexts where it isn't
safe to call __fput_sync().

I can easily see that lazy-unmount is an interesting case which could
easily catch people unawares.  Punting the tail end of mntput_no_expire
(i.e.  if count reaches zero) to a workqueue/task_work makes sense and
would be much less impact than punting every __fput to a workqueue.

Would that make an fput_now() call safe to use in most contexts, or is
there something about ->release or dentry_kill() that can still cause
problems?

Thanks,
NeilBrown


> 
> It really isn't a general-purpose API; any "more friendly name"
> is going to be NAKed for that reason alone.
> 
> Al, very much tempted to send a patch renaming that sucker to
> __fput_dont_use_that_unless_you_really_know_what_you_are_doing().
>