| Message ID | 20240821-work-i_state-v2-4-67244769f102@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | inode: turn i_state into u32 | expand |
On Thu, 22 Aug 2024, Christian Brauner wrote: > Port the __I_NEW mechanism to use the new var event mechanism. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > fs/bcachefs/fs.c | 10 ++++++---- > fs/dcache.c | 3 +-- > fs/inode.c | 18 ++++++++---------- > include/linux/writeback.h | 3 ++- > 4 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c > index 94c392abef65..c0900c0c0f8a 100644 > --- a/fs/bcachefs/fs.c > +++ b/fs/bcachefs/fs.c > @@ -1644,14 +1644,16 @@ void bch2_evict_subvolume_inodes(struct bch_fs *c, snapshot_id_list *s) > break; > } > } else if (clean_pass && this_pass_clean) { > - wait_queue_head_t *wq = bit_waitqueue(&inode->v.i_state, __I_NEW); > - DEFINE_WAIT_BIT(wait, &inode->v.i_state, __I_NEW); > + struct wait_bit_queue_entry wqe; > + struct wait_queue_head *wq_head; > > - prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE); > + wq_head = inode_bit_waitqueue(&wqe, &inode->v, __I_NEW); I don't think you EXPORT inode_bit_waitqueue() so you cannot use it in this module. And maybe it would be good to not export it so that this code can get cleaned up. Maybe I'm missing something obvious but it seems weird. Earlier in this file a comment tells use that bcache doesn't use I_NEW, but here is bcache explicitly waiting for it. If bch2_inode_insert() called unlock_new_inode() immediately *before* adding the inode to vfs_inodes_list instead of just after, this loop that walks vfs_inodes_list would never need to wait for I_NEW to be cleared. I wonder if I am missing something. NeilBrown
On Fri, Aug 23, 2024 at 10:31:55AM GMT, NeilBrown wrote: > On Thu, 22 Aug 2024, Christian Brauner wrote: > > Port the __I_NEW mechanism to use the new var event mechanism. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > fs/bcachefs/fs.c | 10 ++++++---- > > fs/dcache.c | 3 +-- > > fs/inode.c | 18 ++++++++---------- > > include/linux/writeback.h | 3 ++- > > 4 files changed, 17 insertions(+), 17 deletions(-) > > > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c > > index 94c392abef65..c0900c0c0f8a 100644 > > --- a/fs/bcachefs/fs.c > > +++ b/fs/bcachefs/fs.c > > @@ -1644,14 +1644,16 @@ void bch2_evict_subvolume_inodes(struct bch_fs *c, snapshot_id_list *s) > > break; > > } > > } else if (clean_pass && this_pass_clean) { > > - wait_queue_head_t *wq = bit_waitqueue(&inode->v.i_state, __I_NEW); > > - DEFINE_WAIT_BIT(wait, &inode->v.i_state, __I_NEW); > > + struct wait_bit_queue_entry wqe; > > + struct wait_queue_head *wq_head; > > > > - prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE); > > + wq_head = inode_bit_waitqueue(&wqe, &inode->v, __I_NEW); > > I don't think you EXPORT inode_bit_waitqueue() so you cannot use it in > this module. I had already fixed that in-tree because I got a build failure on one of my test machines. > > And maybe it would be good to not export it so that this code can get > cleaned up. Maybe but I prefer this just to be a follow-up. So that we get hung up on ever more details.
On Fri, Aug 23, 2024 at 10:20:31AM GMT, Christian Brauner wrote: > On Fri, Aug 23, 2024 at 10:31:55AM GMT, NeilBrown wrote: > > On Thu, 22 Aug 2024, Christian Brauner wrote: > > > Port the __I_NEW mechanism to use the new var event mechanism. > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > --- > > > fs/bcachefs/fs.c | 10 ++++++---- > > > fs/dcache.c | 3 +-- > > > fs/inode.c | 18 ++++++++---------- > > > include/linux/writeback.h | 3 ++- > > > 4 files changed, 17 insertions(+), 17 deletions(-) > > > > > > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c > > > index 94c392abef65..c0900c0c0f8a 100644 > > > --- a/fs/bcachefs/fs.c > > > +++ b/fs/bcachefs/fs.c > > > @@ -1644,14 +1644,16 @@ void bch2_evict_subvolume_inodes(struct bch_fs *c, snapshot_id_list *s) > > > break; > > > } > > > } else if (clean_pass && this_pass_clean) { > > > - wait_queue_head_t *wq = bit_waitqueue(&inode->v.i_state, __I_NEW); > > > - DEFINE_WAIT_BIT(wait, &inode->v.i_state, __I_NEW); > > > + struct wait_bit_queue_entry wqe; > > > + struct wait_queue_head *wq_head; > > > > > > - prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE); > > > + wq_head = inode_bit_waitqueue(&wqe, &inode->v, __I_NEW); > > > > I don't think you EXPORT inode_bit_waitqueue() so you cannot use it in > > this module. > > I had already fixed that in-tree because I got a build failure on one of > my test machines. > > > > > And maybe it would be good to not export it so that this code can get > > cleaned up. > > Maybe but I prefer this just to be a follow-up. So that we get hung up *don't > on ever more details.
diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c index 94c392abef65..c0900c0c0f8a 100644 --- a/fs/bcachefs/fs.c +++ b/fs/bcachefs/fs.c @@ -1644,14 +1644,16 @@ void bch2_evict_subvolume_inodes(struct bch_fs *c, snapshot_id_list *s) break; } } else if (clean_pass && this_pass_clean) { - wait_queue_head_t *wq = bit_waitqueue(&inode->v.i_state, __I_NEW); - DEFINE_WAIT_BIT(wait, &inode->v.i_state, __I_NEW); + struct wait_bit_queue_entry wqe; + struct wait_queue_head *wq_head; - prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE); + wq_head = inode_bit_waitqueue(&wqe, &inode->v, __I_NEW); + prepare_to_wait_event(wq_head, &wqe.wq_entry, + TASK_UNINTERRUPTIBLE); mutex_unlock(&c->vfs_inodes_lock); schedule(); - finish_wait(wq, &wait.wq_entry); + finish_wait(wq_head, &wqe.wq_entry); goto again; } } diff --git a/fs/dcache.c b/fs/dcache.c index 1af75fa68638..7037f9312ed4 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1908,8 +1908,7 @@ void d_instantiate_new(struct dentry *entry, struct inode *inode) __d_instantiate(entry, inode); WARN_ON(!(inode->i_state & I_NEW)); inode->i_state &= ~I_NEW & ~I_CREATING; - smp_mb(); - wake_up_bit(&inode->i_state, __I_NEW); + inode_wake_up_bit(inode, __I_NEW); spin_unlock(&inode->i_lock); } EXPORT_SYMBOL(d_instantiate_new); diff --git a/fs/inode.c b/fs/inode.c index f2a2f6351ec3..d18e1567c487 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -733,7 +733,7 @@ static void evict(struct inode *inode) * used as an indicator whether blocking on it is safe. */ spin_lock(&inode->i_lock); - wake_up_bit(&inode->i_state, __I_NEW); + inode_wake_up_bit(inode, __I_NEW); BUG_ON(inode->i_state != (I_FREEING | I_CLEAR)); spin_unlock(&inode->i_lock); @@ -1141,8 +1141,7 @@ void unlock_new_inode(struct inode *inode) spin_lock(&inode->i_lock); WARN_ON(!(inode->i_state & I_NEW)); inode->i_state &= ~I_NEW & ~I_CREATING; - smp_mb(); - wake_up_bit(&inode->i_state, __I_NEW); + inode_wake_up_bit(inode, __I_NEW); spin_unlock(&inode->i_lock); } EXPORT_SYMBOL(unlock_new_inode); @@ -1153,8 +1152,7 @@ void discard_new_inode(struct inode *inode) spin_lock(&inode->i_lock); WARN_ON(!(inode->i_state & I_NEW)); inode->i_state &= ~I_NEW; - smp_mb(); - wake_up_bit(&inode->i_state, __I_NEW); + inode_wake_up_bit(inode, __I_NEW); spin_unlock(&inode->i_lock); iput(inode); } @@ -2343,8 +2341,8 @@ EXPORT_SYMBOL(inode_needs_sync); */ static void __wait_on_freeing_inode(struct inode *inode, bool is_inode_hash_locked) { - wait_queue_head_t *wq; - DEFINE_WAIT_BIT(wait, &inode->i_state, __I_NEW); + struct wait_bit_queue_entry wqe; + struct wait_queue_head *wq_head; /* * Handle racing against evict(), see that routine for more details. @@ -2355,14 +2353,14 @@ static void __wait_on_freeing_inode(struct inode *inode, bool is_inode_hash_lock return; } - wq = bit_waitqueue(&inode->i_state, __I_NEW); - prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE); + wq_head = inode_bit_waitqueue(&wqe, inode, __I_NEW); + prepare_to_wait_event(wq_head, &wqe.wq_entry, TASK_UNINTERRUPTIBLE); spin_unlock(&inode->i_lock); rcu_read_unlock(); if (is_inode_hash_locked) spin_unlock(&inode_hash_lock); schedule(); - finish_wait(wq, &wait.wq_entry); + finish_wait(wq_head, &wqe.wq_entry); if (is_inode_hash_locked) spin_lock(&inode_hash_lock); rcu_read_lock(); diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 56b85841ae4c..bed795b8340b 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -200,7 +200,8 @@ void inode_io_list_del(struct inode *inode); /* writeback.h requires fs.h; it, too, is not included from here. */ static inline void wait_on_inode(struct inode *inode) { - wait_on_bit(&inode->i_state, __I_NEW, TASK_UNINTERRUPTIBLE); + wait_var_event(inode_state_wait_address(inode, __I_NEW), + !(inode->i_state & I_NEW)); } #ifdef CONFIG_CGROUP_WRITEBACK
Port the __I_NEW mechanism to use the new var event mechanism. Signed-off-by: Christian Brauner <brauner@kernel.org> --- fs/bcachefs/fs.c | 10 ++++++---- fs/dcache.c | 3 +-- fs/inode.c | 18 ++++++++---------- include/linux/writeback.h | 3 ++- 4 files changed, 17 insertions(+), 17 deletions(-)