Re: [PATCH block/for-4.5-fixes] writeback: keep superblock pinned during cgroup writeback association switches

[Tejun Heo]

Hello,

On Fri, Feb 19, 2016 at 09:58:11PM +0000, Al Viro wrote:
> Um...  What ordering requirements do you have?  You obviously shouldn't
> let it continue past the shutdown - as the matter of fact, you *can't* let
> it continue past generic_shutdown_super(), since any inode references
> held at evict_inodes() time will make it very unhappy.  Attempts to do
> any IO after that will make things a lot worse than unhappy - data structures
> needed to do it might be gone (and if you hold a bit longer, filesystem
> driver itself might very well be gone, along with the functions you were
> going to call).

It can be thought of as an extension of fs writeback operation and
it'd be ideal if it can hold off sb shutdown as on-going writeback
does through holding s_umount.  Unfortunately, that doesn't seem
possible because there's no way to transfer rwsem ownership.

It doesn't generate any IO.  The reason it's done asynchronously is
because the operation requires an RCU grace period.  After the grace
period, it accesses only the generic inode and address_space and the
only time it ends up accessing sb is through the iput call.
Everything else AFAICS doesn't really care whether the underlying sb
is shut down or not.

> Grabbing ->s_active is a seriously bad idea for another reason - in
> a situation when there's only one mount of given fs, plain umount() should
> _not_ return 0 before fs shutdown is over.  Sure, it is possible that there's
> a binding somewhere, or that it's a lazy umount, etc., but those are "you've
> asked for it" situations; having plain umount of e.g. ext3 on a USB stick
> return success before it is safe to pull that stick out is a Bloody Bad Idea,
> for obvious usability reasons.

I see.

> IOW, while fs shutdown may be async, making it *always* async would be a bad
> bug.  And bumping ->s_active does just that.
> 
> I'd go for trylock inside that work + making generic_shutdown_super()
> kill all such works.  I assume that it *can* be abandoned in situation
> when we know that sync_filesystem() is about to be called and that
> said sync_filesystem() won't, in turn, schedule any such works, of course...

I'll make generic_shutdown_super() to kill all such work items.  I
don't think the work item itself would need further locking tho.  Can
you please elaborate why you thought adding trylock to the work would
be necessary?

Thanks.

-- 
tejun