On Wed 03-09-25 17:19:04, Mateusz Guzik wrote: > On Wed, Sep 3, 2025 at 5:16 PM Mateusz Guzik <mjguzik@xxxxxxxxx> wrote: > > > > On Wed, Sep 3, 2025 at 4:03 PM Mark Tinguely <mark.tinguely@xxxxxxxxxx> wrote: > > > > > > On 9/2/25 10:44 PM, Matthew Wilcox wrote: > > > > On Tue, Sep 02, 2025 at 04:54:28PM +0200, Mateusz Guzik wrote: > > > >> Following up on my response to the refcount patchset, here is a churn > > > >> patch to retire I_WILL_FREE. > > > >> > > > >> The only consumer is the drop inode routine in ocfs2. > > > > > > > > If the only consumer is ocfs2 ... then you should cc the ocfs2 people, > > > > right? > > > > > > > > Fair point, I just copy pasted the list from the patchset, too sloppy. > > > > > >> For the life of me I could not figure out if write_inode_now() is legal > > > >> to call in ->evict_inode later and have no means to test, so I devised a > > > >> hack: let the fs set I_FREEING ahead of time. Also note iput_final() > > > >> issues write_inode_now() anyway but only for the !drop case, which is the > > > >> opposite of what is being returned. > > > >> > > > >> One could further hack around it by having ocfs2 return *DON'T* drop but > > > >> also set I_DONTCACHE, which would result in both issuing the write in > > > >> iput_final() and dropping. I think the hack I did implement is cleaner. > > > >> Preferred option is ->evict_inode from ocfs handling the i/o, but per > > > >> the above I don't know how to do it. > > > > > > I am a lurker in this series and ocfs2. My history has been mostly in > > > XFS/CXFS/DMAPI. I removed the other CC entries because I did not want > > > to blast my opinion unnecessaially. > > > > > > > Hello Mark, > > > > This needs the opinion of the vfs folk though, so I'm adding some of > > the cc list back. ;) > > > > > The flushing in ocfs2_drop_inode() predates the I_DONTCACHE addition. > > > IMO, it would be safest and best to maintain to let ocfs2_drop_inode() > > > return 0 and set I_DONTCACHE and let iput_final() do the correct thing. > > > > > > > wow, I'm sorry for really bad case of engrish in the mail. some of it > gets slightly corrected below. > > > For now that would indeed work in the sense of providing the expected > > behavior, but there is the obvious mismatch of the filesystem claiming > > the inode should not be dropped (by returning 0) and but using a side > > indicator to drop it anyway. This looks like a split-brain scenario > > and sooner or later someone is going to complain about it when they do > > other work in iput_final(). If I was maintaining the layer I would > > reject the idea, but if the actual gatekeepers are fine with it... > > > > The absolute best thing to do long run is to move the i/o in > > ->evict_inode, but someone familiar with the APIs here would do the > > needful(tm) and that's not me. > > I mean the best thing to do in the long run is to move the the write > to ->evict_inode, but I don't know how to do it and don't have any > means to test ocfs2 anyway. Hopefully the ocfs2 folk will be willing > to sort this out? So doing inode writeback with I_FREEING set is definitely fine and even necessary in some cases. Now instead of playing games with I_FREEING in ocfs2_drop_inode() (fs code shouldn't really mess with this flag), I'd move write_inode_now() call into ocfs2_evict_inode(). Regarding the move of write_inode_now() into ->evict_inode in general I'm not so sure. It does make some sense and as I mentioned some filesystems need to writeback inode there anyway (because operations in evict_inode() could have dirtied the inode) but it adds a boilerplate code into about 45 filesystems... Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
