Linux: s_op: use .free_inode

[snajpa]

Motivation and Context

#16608

Description

as per Documentation/filesystems/porting.rst:

quote:

**strongly recommended**

take the RCU-delayed parts of ->destroy_inode() into a new method - ->free_inode().  If ->destroy_inode() becomes empty - all the better, just get rid of it.

endquote.

How Has This Been Tested?

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Performance enhancement (non-breaking change which improves efficiency)
• [ ] Code cleanup (non-breaking change which makes code smaller or more readable)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• [ ] Documentation (a change to man pages or other documentation)

Checklist:

• [x] My code follows the OpenZFS code style requirements.
• [ ] I have updated the documentation accordingly.
• [x] I have read the contributing document.
• [ ] I have added tests to cover my changes.
• [x] I have run the ZFS Test Suite with this change applied.
• [x] All commit messages are properly formatted and contain Signed-off-by.

[robn]

Is there a clear thing that this solves on its own? I'm not saying there's not, but free_inode was introduced in torvalds/linux@fdb0da89f4ba for the 5.2 series, and I don't see any interesting change in "compat" support for filesystems that don't support it, so I'm wondering if this is just a cleanup or actually changes something?

(I'm not against the change as such, I'm mostly just trying to rule it in/out of a customer issue I'm looking at).

[snajpa]

Is there a clear thing that this solves on its own?

https://github.com/openzfs/zfs/issues/16608

We are running with this in prod, btw, the GPFs are gone. Not that 2.3.x wouldn't crash in other ways too, but this was a frequent cause of downtime for us.

Something related must have changed in subsequent releases way later, I would say somewhere at 6.10+, because with 6.9, we weren't getting these GPFs.

[robn]

@snajpa thanks. I wasn't sure; there's a few different things mentioned in that PR.

[amotin]

Seems this was duplicated by #17546, that just landed. I'm sorry this PR was lost.

[behlendorf]

@snajpa sorry I lost track of this one.

[snajpa]

@amotin @behlendorf not very nice, especially given that the merged fix is in my view incomplete

[behlendorf]

Yes you're right, let me apologize again. I should have followed up with you months ago so we could wrap this one up.

As for the merged fix being incomplete, I think we're in good shape. The porting notes do recommend dropping ->destroy_inode() but I'd like to keep it, at least for now. It's still used by several other kernel filesystems and I don't see any gotchas there. Is there something else I'm missing? What are your remaining concerns?

We definitely do still have to pull in your truncate_inode_pages_final change from #16772.

[snajpa]

What are your remaining concerns?

If there was actually anyone in the project, who would read porting.rst, locking.rst and vfs.rst, in the kernel tree in Documentation, as they evolve, this thing would have been caught early on as the kernel, which introduced the change, came out.

It's always documented there - and now the docs are saying, without stating why, that it is for the best to get rid of the delete inode hook completely. Who am I to argue with that, so that's exactly what I did. It says it like it will come to bite - and since this thing took some time to figure out and debugging it wasn't exactly straightforward, I would like to avoid having to spend the time on this thing again in the future.

Re: #16772, this is how it looks like currently: https://github.com/vpsfreecz/zfs/commit/fe92f03d204035d0543bda46fd54ca75a007dce6, just a fast FYI, will update the PR in the next few days, the good news from that is that remove_inode_hash is now gone completely, inode_dec_link_count+discard_new_inode in the error paths seems to be the right way to resolve any issues there might have been with insert -> remove_inode_hash

[amotin]

not very nice, especially given that the merged fix is in my view incomplete

What would be nice is to have more of regular reviewers, so that things would not get lost. This is a quick top 10 reviewers this year to whom I am grateful:

  10     Reviewed-by: Ameer Hamza 
  16     Reviewed-by: Pavel Snajdr 
  18     Reviewed-by: Allan Jude 
  20     Reviewed-by: Paul Dagnelie 
  27     Reviewed-by: George Melikov 
  34     Reviewed-by: Tino Reichardt 
  46     Reviewed-by: Rob Norris 
 130     Reviewed-by: Tony Hutter 
 173     Reviewed-by: Brian Behlendorf 
 209     Reviewed-by: Alexander Motin