| Message ID | 20210802185130.94783-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: new helper - ceph_change_snap_realm | expand |
Jeff Layton <jlayton@kernel.org> writes: > Consolidate some fiddly code for changing an inode's snap_realm > into a new helper function, and change the callers to use it. > > While we're in here, nothing uses the i_snap_realm_counter field, so > remove that from the inode. Ah, nice! I remember _long_ time ago I had seen that field and thought: "I'll need to send out a patch to remove this thing!". But in the meantime I completely forgot. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/caps.c | 36 +++--------------------------- > fs/ceph/inode.c | 11 ++------- > fs/ceph/snap.c | 59 ++++++++++++++++++++++++++++++++----------------- > fs/ceph/super.h | 2 +- > 4 files changed, 45 insertions(+), 63 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index cb551c9e5867..cecd4f66a60d 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -704,23 +704,7 @@ void ceph_add_cap(struct inode *inode, > struct ceph_snap_realm *realm = ceph_lookup_snap_realm(mdsc, > realmino); > if (realm) { > - struct ceph_snap_realm *oldrealm = ci->i_snap_realm; > - if (oldrealm) { > - spin_lock(&oldrealm->inodes_with_caps_lock); > - list_del_init(&ci->i_snap_realm_item); > - spin_unlock(&oldrealm->inodes_with_caps_lock); > - } > - > - spin_lock(&realm->inodes_with_caps_lock); > - list_add(&ci->i_snap_realm_item, > - &realm->inodes_with_caps); > - ci->i_snap_realm = realm; > - if (realm->ino == ci->i_vino.ino) > - realm->inode = inode; > - spin_unlock(&realm->inodes_with_caps_lock); > - > - if (oldrealm) > - ceph_put_snap_realm(mdsc, oldrealm); > + ceph_change_snap_realm(inode, realm); > } else { > pr_err("ceph_add_cap: couldn't find snap realm %llx\n", > realmino); > @@ -1112,20 +1096,6 @@ int ceph_is_any_caps(struct inode *inode) > return ret; > } > > -static void drop_inode_snap_realm(struct ceph_inode_info *ci) > -{ > - struct ceph_snap_realm *realm = ci->i_snap_realm; > - spin_lock(&realm->inodes_with_caps_lock); > - list_del_init(&ci->i_snap_realm_item); > - ci->i_snap_realm_counter++; > - ci->i_snap_realm = NULL; > - if (realm->ino == ci->i_vino.ino) > - realm->inode = NULL; > - spin_unlock(&realm->inodes_with_caps_lock); > - ceph_put_snap_realm(ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc, > - realm); > -} > - > /* > * Remove a cap. Take steps to deal with a racing iterate_session_caps. > * > @@ -1201,7 +1171,7 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release) > * keep i_snap_realm. > */ > if (ci->i_wr_ref == 0 && ci->i_snap_realm) > - drop_inode_snap_realm(ci); > + ceph_change_snap_realm(&ci->vfs_inode, NULL); > > __cap_delay_cancel(mdsc, ci); > } > @@ -3083,7 +3053,7 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had, > } > /* see comment in __ceph_remove_cap() */ > if (!__ceph_is_any_real_caps(ci) && ci->i_snap_realm) > - drop_inode_snap_realm(ci); > + ceph_change_snap_realm(inode, NULL); > } > } > if (check_flushsnaps && __ceph_have_pending_cap_snap(ci)) { > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > index 84e4f112fc45..61ecf81ed875 100644 > --- a/fs/ceph/inode.c > +++ b/fs/ceph/inode.c > @@ -582,16 +582,9 @@ void ceph_evict_inode(struct inode *inode) > */ > if (ci->i_snap_realm) { > if (ceph_snap(inode) == CEPH_NOSNAP) { > - struct ceph_snap_realm *realm = ci->i_snap_realm; > dout(" dropping residual ref to snap realm %p\n", > - realm); > - spin_lock(&realm->inodes_with_caps_lock); > - list_del_init(&ci->i_snap_realm_item); > - ci->i_snap_realm = NULL; > - if (realm->ino == ci->i_vino.ino) > - realm->inode = NULL; > - spin_unlock(&realm->inodes_with_caps_lock); > - ceph_put_snap_realm(mdsc, realm); > + ci->i_snap_realm); > + ceph_change_snap_realm(inode, NULL); > } else { > ceph_put_snapid_map(mdsc, ci->i_snapid_map); > ci->i_snap_realm = NULL; > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c > index 4ac0606dcbd4..9dbc92cfda38 100644 > --- a/fs/ceph/snap.c > +++ b/fs/ceph/snap.c > @@ -846,6 +846,43 @@ static void flush_snaps(struct ceph_mds_client *mdsc) > dout("flush_snaps done\n"); > } > > +/** > + * ceph_change_snap_realm - change the snap_realm for an inode > + * @inode: inode to move to new snap realm > + * @realm: new realm to move inode into (may be NULL) > + * > + * Detach an inode from its old snaprealm (if any) and attach it to > + * the new snaprealm (if any). The old snap realm reference held by > + * the inode is put. If realm is non-NULL, then the caller's reference > + * to it is taken over by the inode. > + */ > +void ceph_change_snap_realm(struct inode *inode, struct ceph_snap_realm *realm) > +{ > + struct ceph_inode_info *ci = ceph_inode(inode); Just a suggestion: this function could received the struct ceph_inode_info instead of the inode. Other than that, LGTM. Nice cleanup! Feel free to add my Reviewed-by: Luis Henriques <lhenriques@suse.de> (The other 2 patches ("print more information when..." and "remove redundant initializations...") also look good BTW.) Cheers
On Tue, 2021-08-03 at 11:21 +0100, Luis Henriques wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > Consolidate some fiddly code for changing an inode's snap_realm > > into a new helper function, and change the callers to use it. > > > > While we're in here, nothing uses the i_snap_realm_counter field, so > > remove that from the inode. > > Ah, nice! I remember _long_ time ago I had seen that field and thought: > "I'll need to send out a patch to remove this thing!". But in the > meantime I completely forgot. > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/caps.c | 36 +++--------------------------- > > fs/ceph/inode.c | 11 ++------- > > fs/ceph/snap.c | 59 ++++++++++++++++++++++++++++++++----------------- > > fs/ceph/super.h | 2 +- > > 4 files changed, 45 insertions(+), 63 deletions(-) > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > index cb551c9e5867..cecd4f66a60d 100644 > > --- a/fs/ceph/caps.c > > +++ b/fs/ceph/caps.c > > @@ -704,23 +704,7 @@ void ceph_add_cap(struct inode *inode, > > struct ceph_snap_realm *realm = ceph_lookup_snap_realm(mdsc, > > realmino); > > if (realm) { > > - struct ceph_snap_realm *oldrealm = ci->i_snap_realm; > > - if (oldrealm) { > > - spin_lock(&oldrealm->inodes_with_caps_lock); > > - list_del_init(&ci->i_snap_realm_item); > > - spin_unlock(&oldrealm->inodes_with_caps_lock); > > - } > > - > > - spin_lock(&realm->inodes_with_caps_lock); > > - list_add(&ci->i_snap_realm_item, > > - &realm->inodes_with_caps); > > - ci->i_snap_realm = realm; > > - if (realm->ino == ci->i_vino.ino) > > - realm->inode = inode; > > - spin_unlock(&realm->inodes_with_caps_lock); > > - > > - if (oldrealm) > > - ceph_put_snap_realm(mdsc, oldrealm); > > + ceph_change_snap_realm(inode, realm); > > } else { > > pr_err("ceph_add_cap: couldn't find snap realm %llx\n", > > realmino); > > @@ -1112,20 +1096,6 @@ int ceph_is_any_caps(struct inode *inode) > > return ret; > > } > > > > -static void drop_inode_snap_realm(struct ceph_inode_info *ci) > > -{ > > - struct ceph_snap_realm *realm = ci->i_snap_realm; > > - spin_lock(&realm->inodes_with_caps_lock); > > - list_del_init(&ci->i_snap_realm_item); > > - ci->i_snap_realm_counter++; > > - ci->i_snap_realm = NULL; > > - if (realm->ino == ci->i_vino.ino) > > - realm->inode = NULL; > > - spin_unlock(&realm->inodes_with_caps_lock); > > - ceph_put_snap_realm(ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc, > > - realm); > > -} > > - > > /* > > * Remove a cap. Take steps to deal with a racing iterate_session_caps. > > * > > @@ -1201,7 +1171,7 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release) > > * keep i_snap_realm. > > */ > > if (ci->i_wr_ref == 0 && ci->i_snap_realm) > > - drop_inode_snap_realm(ci); > > + ceph_change_snap_realm(&ci->vfs_inode, NULL); > > > > __cap_delay_cancel(mdsc, ci); > > } > > @@ -3083,7 +3053,7 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had, > > } > > /* see comment in __ceph_remove_cap() */ > > if (!__ceph_is_any_real_caps(ci) && ci->i_snap_realm) > > - drop_inode_snap_realm(ci); > > + ceph_change_snap_realm(inode, NULL); > > } > > } > > if (check_flushsnaps && __ceph_have_pending_cap_snap(ci)) { > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c > > index 84e4f112fc45..61ecf81ed875 100644 > > --- a/fs/ceph/inode.c > > +++ b/fs/ceph/inode.c > > @@ -582,16 +582,9 @@ void ceph_evict_inode(struct inode *inode) > > */ > > if (ci->i_snap_realm) { > > if (ceph_snap(inode) == CEPH_NOSNAP) { > > - struct ceph_snap_realm *realm = ci->i_snap_realm; > > dout(" dropping residual ref to snap realm %p\n", > > - realm); > > - spin_lock(&realm->inodes_with_caps_lock); > > - list_del_init(&ci->i_snap_realm_item); > > - ci->i_snap_realm = NULL; > > - if (realm->ino == ci->i_vino.ino) > > - realm->inode = NULL; > > - spin_unlock(&realm->inodes_with_caps_lock); > > - ceph_put_snap_realm(mdsc, realm); > > + ci->i_snap_realm); > > + ceph_change_snap_realm(inode, NULL); > > } else { > > ceph_put_snapid_map(mdsc, ci->i_snapid_map); > > ci->i_snap_realm = NULL; > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c > > index 4ac0606dcbd4..9dbc92cfda38 100644 > > --- a/fs/ceph/snap.c > > +++ b/fs/ceph/snap.c > > @@ -846,6 +846,43 @@ static void flush_snaps(struct ceph_mds_client *mdsc) > > dout("flush_snaps done\n"); > > } > > > > +/** > > + * ceph_change_snap_realm - change the snap_realm for an inode > > + * @inode: inode to move to new snap realm > > + * @realm: new realm to move inode into (may be NULL) > > + * > > + * Detach an inode from its old snaprealm (if any) and attach it to > > + * the new snaprealm (if any). The old snap realm reference held by > > + * the inode is put. If realm is non-NULL, then the caller's reference > > + * to it is taken over by the inode. > > + */ > > +void ceph_change_snap_realm(struct inode *inode, struct ceph_snap_realm *realm) > > +{ > > + struct ceph_inode_info *ci = ceph_inode(inode); > > Just a suggestion: this function could received the struct > ceph_inode_info instead of the inode. Other than that, LGTM. Nice > cleanup! Feel free to add my > > Reviewed-by: Luis Henriques <lhenriques@suse.de> > Meh -- I started with that, but then I needed to get to the inode itself for the realm->inode and to get to the mdsc. I'm inclined to leave it as-is since I've already tested this version, but maybe we could change it later if we need to add new callers. Thanks for the Reviewed-by though! > (The other 2 patches ("print more information when..." and "remove > redundant initializations...") also look good BTW.) > > Cheers
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index cb551c9e5867..cecd4f66a60d 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -704,23 +704,7 @@ void ceph_add_cap(struct inode *inode, struct ceph_snap_realm *realm = ceph_lookup_snap_realm(mdsc, realmino); if (realm) { - struct ceph_snap_realm *oldrealm = ci->i_snap_realm; - if (oldrealm) { - spin_lock(&oldrealm->inodes_with_caps_lock); - list_del_init(&ci->i_snap_realm_item); - spin_unlock(&oldrealm->inodes_with_caps_lock); - } - - spin_lock(&realm->inodes_with_caps_lock); - list_add(&ci->i_snap_realm_item, - &realm->inodes_with_caps); - ci->i_snap_realm = realm; - if (realm->ino == ci->i_vino.ino) - realm->inode = inode; - spin_unlock(&realm->inodes_with_caps_lock); - - if (oldrealm) - ceph_put_snap_realm(mdsc, oldrealm); + ceph_change_snap_realm(inode, realm); } else { pr_err("ceph_add_cap: couldn't find snap realm %llx\n", realmino); @@ -1112,20 +1096,6 @@ int ceph_is_any_caps(struct inode *inode) return ret; } -static void drop_inode_snap_realm(struct ceph_inode_info *ci) -{ - struct ceph_snap_realm *realm = ci->i_snap_realm; - spin_lock(&realm->inodes_with_caps_lock); - list_del_init(&ci->i_snap_realm_item); - ci->i_snap_realm_counter++; - ci->i_snap_realm = NULL; - if (realm->ino == ci->i_vino.ino) - realm->inode = NULL; - spin_unlock(&realm->inodes_with_caps_lock); - ceph_put_snap_realm(ceph_sb_to_client(ci->vfs_inode.i_sb)->mdsc, - realm); -} - /* * Remove a cap. Take steps to deal with a racing iterate_session_caps. * @@ -1201,7 +1171,7 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release) * keep i_snap_realm. */ if (ci->i_wr_ref == 0 && ci->i_snap_realm) - drop_inode_snap_realm(ci); + ceph_change_snap_realm(&ci->vfs_inode, NULL); __cap_delay_cancel(mdsc, ci); } @@ -3083,7 +3053,7 @@ static void __ceph_put_cap_refs(struct ceph_inode_info *ci, int had, } /* see comment in __ceph_remove_cap() */ if (!__ceph_is_any_real_caps(ci) && ci->i_snap_realm) - drop_inode_snap_realm(ci); + ceph_change_snap_realm(inode, NULL); } } if (check_flushsnaps && __ceph_have_pending_cap_snap(ci)) { diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index 84e4f112fc45..61ecf81ed875 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -582,16 +582,9 @@ void ceph_evict_inode(struct inode *inode) */ if (ci->i_snap_realm) { if (ceph_snap(inode) == CEPH_NOSNAP) { - struct ceph_snap_realm *realm = ci->i_snap_realm; dout(" dropping residual ref to snap realm %p\n", - realm); - spin_lock(&realm->inodes_with_caps_lock); - list_del_init(&ci->i_snap_realm_item); - ci->i_snap_realm = NULL; - if (realm->ino == ci->i_vino.ino) - realm->inode = NULL; - spin_unlock(&realm->inodes_with_caps_lock); - ceph_put_snap_realm(mdsc, realm); + ci->i_snap_realm); + ceph_change_snap_realm(inode, NULL); } else { ceph_put_snapid_map(mdsc, ci->i_snapid_map); ci->i_snap_realm = NULL; diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c index 4ac0606dcbd4..9dbc92cfda38 100644 --- a/fs/ceph/snap.c +++ b/fs/ceph/snap.c @@ -846,6 +846,43 @@ static void flush_snaps(struct ceph_mds_client *mdsc) dout("flush_snaps done\n"); } +/** + * ceph_change_snap_realm - change the snap_realm for an inode + * @inode: inode to move to new snap realm + * @realm: new realm to move inode into (may be NULL) + * + * Detach an inode from its old snaprealm (if any) and attach it to + * the new snaprealm (if any). The old snap realm reference held by + * the inode is put. If realm is non-NULL, then the caller's reference + * to it is taken over by the inode. + */ +void ceph_change_snap_realm(struct inode *inode, struct ceph_snap_realm *realm) +{ + struct ceph_inode_info *ci = ceph_inode(inode); + struct ceph_mds_client *mdsc = ceph_inode_to_client(inode)->mdsc; + struct ceph_snap_realm *oldrealm = ci->i_snap_realm; + + lockdep_assert_held(&ci->i_ceph_lock); + + if (oldrealm) { + spin_lock(&oldrealm->inodes_with_caps_lock); + list_del_init(&ci->i_snap_realm_item); + if (oldrealm->ino == ci->i_vino.ino) + oldrealm->inode = NULL; + spin_unlock(&oldrealm->inodes_with_caps_lock); + ceph_put_snap_realm(mdsc, oldrealm); + } + + ci->i_snap_realm = realm; + + if (realm) { + spin_lock(&realm->inodes_with_caps_lock); + list_add(&ci->i_snap_realm_item, &realm->inodes_with_caps); + if (realm->ino == ci->i_vino.ino) + realm->inode = inode; + spin_unlock(&realm->inodes_with_caps_lock); + } +} /* * Handle a snap notification from the MDS. @@ -932,7 +969,6 @@ void ceph_handle_snap(struct ceph_mds_client *mdsc, }; struct inode *inode = ceph_find_inode(sb, vino); struct ceph_inode_info *ci; - struct ceph_snap_realm *oldrealm; if (!inode) continue; @@ -957,27 +993,10 @@ void ceph_handle_snap(struct ceph_mds_client *mdsc, } dout(" will move %p to split realm %llx %p\n", inode, realm->ino, realm); - /* - * Move the inode to the new realm - */ - oldrealm = ci->i_snap_realm; - spin_lock(&oldrealm->inodes_with_caps_lock); - list_del_init(&ci->i_snap_realm_item); - spin_unlock(&oldrealm->inodes_with_caps_lock); - - spin_lock(&realm->inodes_with_caps_lock); - list_add(&ci->i_snap_realm_item, - &realm->inodes_with_caps); - ci->i_snap_realm = realm; - if (realm->ino == ci->i_vino.ino) - realm->inode = inode; - spin_unlock(&realm->inodes_with_caps_lock); - - spin_unlock(&ci->i_ceph_lock); ceph_get_snap_realm(mdsc, realm); - ceph_put_snap_realm(mdsc, oldrealm); - + ceph_change_snap_realm(inode, realm); + spin_unlock(&ci->i_ceph_lock); iput(inode); continue; diff --git a/fs/ceph/super.h b/fs/ceph/super.h index d51d42a00f33..389b45ac291b 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -422,7 +422,6 @@ struct ceph_inode_info { struct ceph_snap_realm *i_snap_realm; /* snap realm (if caps) */ struct ceph_snapid_map *i_snapid_map; /* snapid -> dev_t */ }; - int i_snap_realm_counter; /* snap realm (if caps) */ struct list_head i_snap_realm_item; struct list_head i_snap_flush_item; struct timespec64 i_btime; @@ -933,6 +932,7 @@ extern void ceph_put_snap_realm(struct ceph_mds_client *mdsc, extern int ceph_update_snap_trace(struct ceph_mds_client *m, void *p, void *e, bool deletion, struct ceph_snap_realm **realm_ret); +void ceph_change_snap_realm(struct inode *inode, struct ceph_snap_realm *realm); extern void ceph_handle_snap(struct ceph_mds_client *mdsc, struct ceph_mds_session *session, struct ceph_msg *msg);
Consolidate some fiddly code for changing an inode's snap_realm into a new helper function, and change the callers to use it. While we're in here, nothing uses the i_snap_realm_counter field, so remove that from the inode. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/caps.c | 36 +++--------------------------- fs/ceph/inode.c | 11 ++------- fs/ceph/snap.c | 59 ++++++++++++++++++++++++++++++++----------------- fs/ceph/super.h | 2 +- 4 files changed, 45 insertions(+), 63 deletions(-)