On Wed, Apr 10, 2019 at 05:31:35AM +0100, Al Viro wrote: > On Tue, Apr 09, 2019 at 09:04:15PM -0700, Eric Biggers wrote: > > > > What's to stop you from doing just that right now? You'd need to take > > > care with barriers, but you'd need that anyway... As soon as ->i_link is set > > > you'll get no more ->get_link() on that sucker, using the cached value > > > from that point on. IDGI... > > > > 1.) The VFS won't know to drop of RCU-walk mode, so waiting an RCU grace period > > before freeing the symlink target becomes mandatory. (Which I'd like to do > > for fscrypt anyway, but doing it sanely appears to require implementing > > .destroy_inode() for ext4, f2fs, and ubifs. I hoped I could do non-RCU mode > > as a simpler first step.) > > You might want to check those filesystems. All three you've mentioned *have* > ->destroy_inode() already. > Yep, I just noticed that. > > 2.) The VFS won't know to use a read memory barrier when loading i_link. > > The VFS could issue one unconditionally, but it would be unnecessary for > > regular fast symlinks. > > Not really. All we need on the read side is READ_ONCE(); it will supply > smp_read_barrier_depends() (which is a no-op except for alpha). On the > write side we need smp_store_release() to set ->i_link (in addition to > whatever serialization we want for actual calculation of the value to > be cached, of course). Okay, I didn't realize that READ_ONCE() would be sufficient. I thought smp_load_acquire() was needed. I guess you're right; we'd only read what the pointer points to, so it's a data dependency. Do you see any problem with using cmpxchg_release() on the write side, so no additional lock is needed? (Like what we do for ->i_crypt_info, except currently it's actually cmpxchg() there, with a direct access on the read side. IIUC now, that should be changed to cmpxchg_release() and READ_ONCE().) - Eric
