On Mon, Mar 25, 2019 at 02:45:00PM -0700, Linus Torvalds wrote: > On Mon, Mar 25, 2019 at 2:14 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > > > Maybe, but we really need to come up with sane documentation on the > > entire drop_inode/evict_inode/destroy_inode/rcu_destroy_inode > > group ;-/ > > Yeah. > > I actually think the "destroy_inode/rcu_destroy_inode" part is the > simplest one to understand: it's just tearing down the inode, and the > RCU part is (obviously, as shown by this thread) somewhat subtle, but > at the same time not really all that hard to explain: "inodes that can > be looked up through RCU lookup need to be destroyed with RCU delay". ... with at least "but remember that call_rcu()-scheduled stuff runs in softirq, so there's a limit to what you can defer there; furthermore, there are implications for any spinlocks taken in that callback". > I think drop_inode->evict_inode is arguably the much more subtle > stuff. That's the "we're not destroying things, we're making decisions > about the state" kind of thing. It's worse than that - another bloody subtle question is the location of clear_inode() wrt the other work done in ->evict_inode() ;-/ > And we actually don't have any documentation at all for those two. > Well, there's the "porting" thing, but.. > > > And I want to understand the writeback-related issues > > in ocfs2 and f2fs - the current kludges in those smell fishy. > > I'm assuming you're talking about exactly that drop_inode() kind of subtlety. Yes. > At least for ocfs2, the whole "rcu_destroy_inode()" patch I sent out > would simplify things a lot. *All* that the ocfs2_destroy_inode() > function does is to schedule the inode freeing for RCU delay. > > Assuming my patch is fine (as mentioned, it was entirely untested), > that patch would actually simplify that a lot. Get rid of > ocfs2_destroy_inode() entirely, and just make > ocfs2_rcu_destroy_inode() do that kmem_cache_free(). Mucho clean-up > (we have that ocfs2_rcu_destroy_inode() currently as > ocfs2_i_callback(), but with the ugly rcu-head interface). Sure, but I wonder if cleaner solution (if any) for whatever is going on in their drop_inode/evict_inode area might spill into destroy_inode... I certainly agree that having destroy_inode consist just of call_rcu() is very common. FWIW, non-trivial exceptions from that pattern are: fs/afs/super.c:673:static void afs_destroy_inode(struct inode *inode) fs/btrfs/inode.c:9215:void btrfs_destroy_inode(struct inode *inode) fs/btrfs/inode.c:9202:void btrfs_test_destroy_inode(struct inode *inode) fs/ceph/inode.c:530:void ceph_destroy_inode(struct inode *inode) fs/ecryptfs/super.c:88:static void ecryptfs_destroy_inode(struct inode *inode) fs/ext4/super.c:1106:static void ext4_destroy_inode(struct inode *inode) fs/inode.c:228:void free_inode_nonrcu(struct inode *inode) fs/fuse/inode.c:116:static void fuse_destroy_inode(struct inode *inode) fs/hugetlbfs/inode.c:1052:static void hugetlbfs_destroy_inode(struct inode *inode) fs/jfs/super.c:134:static void jfs_destroy_inode(struct inode *inode) fs/ntfs/inode.c:341:void ntfs_destroy_big_inode(struct inode *inode) fs/overlayfs/super.c:200:static void ovl_destroy_inode(struct inode *inode) mm/shmem.c:3646:static void shmem_destroy_inode(struct inode *inode) net/socket.c:266:static void sock_destroy_inode(struct inode *inode) fs/ubifs/super.c:282:static void ubifs_destroy_inode(struct inode *inode) fs/xfs/xfs_super.c:969:xfs_fs_destroy_inode( Some of those could convert to that pattern, so it's even fewer than that. And getting rid of container_of in those callbacks would be nice too. One obvious observation, though - we want the actual fixes to go before any method changes, for backporting reasons. For debugfs it's clearly "use default ->evict_inode(), have explicit ->destroy_inode() using free_inode_nonrcu()" - there we have nothing else done in ->evict_inode() and kfree is obviously safe in softirq. I'll post that (or push to vfs.git#fixes), along with minimal fixes for other 3. If bpf_any_put() is softirq-safe, we'll have the full set for -stable and the rest could be done on top of that. Won't solve the documetation problem, unfortunately ;-/
