| Message ID | 20231124060422.576198-2-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,01/21] switch nfsd_client_rmdir() to use of simple_recursive_removal() | expand |
On Thu, 23 Nov 2023 at 22:04, Al Viro <viro@zeniv.linux.org.uk> wrote: > > ->d_lock on parent does not stabilize ->d_inode of child. > We don't do much with that inode in there, but we need > at least to avoid struct inode getting freed under us... Gaah. We've gone back and forth on this. Being non-preemptible is already equivalent to rcu read locking. From Documentation/RCU/rcu_dereference.rst: With the new consolidated RCU flavors, an RCU read-side critical section is entered using rcu_read_lock(), anything that disables bottom halves, anything that disables interrupts, or anything that disables preemption. so I actually think the coda code is already mostly fine, because that parent spin_lock may not stabilize d_child per se, but it *does* imply a RCU read lock. So I think you should drop the rcu_read_lock/rcu_read_unlock from that patch. But that struct inode *inode = d_inode_rcu(de); conversion is required to get a stable inode pointer. So half of this patch is unnecessary. Adding Paul to the cc just to verify that the docs are up-to-date and that we're still good here. Because we've gone back-and-forth on the "spinlocks are an implied RCU read-side critical section" a couple of times. Linus
On Fri, Nov 24, 2023 at 01:22:19PM -0800, Linus Torvalds wrote: > On Thu, 23 Nov 2023 at 22:04, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > ->d_lock on parent does not stabilize ->d_inode of child. > > We don't do much with that inode in there, but we need > > at least to avoid struct inode getting freed under us... > > Gaah. We've gone back and forth on this. Being non-preemptible is > already equivalent to rcu read locking. > > >From Documentation/RCU/rcu_dereference.rst: > > With the new consolidated > RCU flavors, an RCU read-side critical section is entered > using rcu_read_lock(), anything that disables bottom halves, > anything that disables interrupts, or anything that disables > preemption. > > so I actually think the coda code is already mostly fine, because that > parent spin_lock may not stabilize d_child per se, but it *does* imply > a RCU read lock. > > So I think you should drop the rcu_read_lock/rcu_read_unlock from that patch. > > But that > > struct inode *inode = d_inode_rcu(de); > > conversion is required to get a stable inode pointer. > > So half of this patch is unnecessary. > > Adding Paul to the cc just to verify that the docs are up-to-date and > that we're still good here. > > Because we've gone back-and-forth on the "spinlocks are an implied RCU > read-side critical section" a couple of times. Yes, spinlocks are implied RCU read-side critical sections. Even in -rt, where non-raw spinlocks are preemptible, courtesy of this: static __always_inline void __rt_spin_lock(spinlock_t *lock) { rtlock_might_resched(); rtlock_lock(&lock->lock); rcu_read_lock(); migrate_disable(); } So given -rt's preemptible spinlocks still being RCU readers, I need to explicitly call this out in the documentation. How about as shown below for a start? Thanx, Paul ------------------------------------------------------------------------ diff --git a/Documentation/RCU/rcu_dereference.rst b/Documentation/RCU/rcu_dereference.rst index 659d5913784d..2524dcdadde2 100644 --- a/Documentation/RCU/rcu_dereference.rst +++ b/Documentation/RCU/rcu_dereference.rst @@ -408,7 +408,10 @@ member of the rcu_dereference() to use in various situations: RCU flavors, an RCU read-side critical section is entered using rcu_read_lock(), anything that disables bottom halves, anything that disables interrupts, or anything that disables - preemption. + preemption. Please note that spinlock critical sections + are also implied RCU read-side critical sections, even when + they are preemptible, as they are in kernels built with + CONFIG_PREEMPT_RT=y. 2. If the access might be within an RCU read-side critical section on the one hand, or protected by (say) my_lock on the other,
diff --git a/fs/coda/cache.c b/fs/coda/cache.c index 3b8c4513118f..bfbc03c6b632 100644 --- a/fs/coda/cache.c +++ b/fs/coda/cache.c @@ -92,13 +92,16 @@ static void coda_flag_children(struct dentry *parent, int flag) { struct dentry *de; + rcu_read_lock(); spin_lock(&parent->d_lock); list_for_each_entry(de, &parent->d_subdirs, d_child) { + struct inode *inode = d_inode_rcu(de); /* don't know what to do with negative dentries */ - if (d_inode(de) ) - coda_flag_inode(d_inode(de), flag); + if (inode) + coda_flag_inode(inode, flag); } spin_unlock(&parent->d_lock); + rcu_read_unlock(); return; }