| Message ID | 20201007122536.13354-1-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: break up send_cap_msg | expand |
On Wed, Oct 7, 2020 at 2:25 PM Jeff Layton <jlayton@kernel.org> wrote: > > Push the allocation of the msg and the send into the caller. Rename > the function to marshal_cap_msg and make it void return. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/caps.c | 61 +++++++++++++++++++++++++------------------------- > 1 file changed, 30 insertions(+), 31 deletions(-) > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > index 4e84b39a6ebd..6f4adfaf761f 100644 > --- a/fs/ceph/caps.c > +++ b/fs/ceph/caps.c > @@ -1222,36 +1222,29 @@ struct cap_msg_args { > }; > > /* > - * Build and send a cap message to the given MDS. > - * > - * Caller should be holding s_mutex. > + * cap struct size + flock buffer size + inline version + inline data size + > + * osd_epoch_barrier + oldest_flush_tid > */ > -static int send_cap_msg(struct cap_msg_args *arg) > +#define CAP_MSG_SIZE (sizeof(struct ceph_mds_caps) + \ > + 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4) > + > +/* Marshal up the cap msg to the MDS */ > +static void marshal_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg) Nit: functions like this usually have "encode" or "build" in their names across the codebase, so I'd go with "encode_cap_msg". > { > struct ceph_mds_caps *fc; > - struct ceph_msg *msg; > void *p; > - size_t extra_len; > struct ceph_osd_client *osdc = &arg->session->s_mdsc->fsc->client->osdc; > > - dout("send_cap_msg %s %llx %llx caps %s wanted %s dirty %s" > + dout("%s %s %llx %llx caps %s wanted %s dirty %s" > " seq %u/%u tid %llu/%llu mseq %u follows %lld size %llu/%llu" > - " xattr_ver %llu xattr_len %d\n", ceph_cap_op_name(arg->op), > - arg->cid, arg->ino, ceph_cap_string(arg->caps), > - ceph_cap_string(arg->wanted), ceph_cap_string(arg->dirty), > - arg->seq, arg->issue_seq, arg->flush_tid, arg->oldest_flush_tid, > - arg->mseq, arg->follows, arg->size, arg->max_size, > - arg->xattr_version, > + " xattr_ver %llu xattr_len %d\n", __func__, > + ceph_cap_op_name(arg->op), arg->cid, arg->ino, > + ceph_cap_string(arg->caps), ceph_cap_string(arg->wanted), > + ceph_cap_string(arg->dirty), arg->seq, arg->issue_seq, > + arg->flush_tid, arg->oldest_flush_tid, arg->mseq, arg->follows, > + arg->size, arg->max_size, arg->xattr_version, > arg->xattr_buf ? (int)arg->xattr_buf->vec.iov_len : 0); > > - /* flock buffer size + inline version + inline data size + > - * osd_epoch_barrier + oldest_flush_tid */ > - extra_len = 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4; > - msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, sizeof(*fc) + extra_len, > - GFP_NOFS, false); > - if (!msg) > - return -ENOMEM; > - > msg->hdr.version = cpu_to_le16(10); > msg->hdr.tid = cpu_to_le64(arg->flush_tid); > > @@ -1323,9 +1316,6 @@ static int send_cap_msg(struct cap_msg_args *arg) > > /* Advisory flags (version 10) */ > ceph_encode_32(&p, arg->flags); > - > - ceph_con_send(&arg->session->s_con, msg); > - return 0; > } > > /* > @@ -1456,22 +1446,24 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap, > */ > static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci) > { > + struct ceph_msg *msg; > struct inode *inode = &ci->vfs_inode; > - int ret; > > - ret = send_cap_msg(arg); > - if (ret < 0) { > - pr_err("error sending cap msg, ino (%llx.%llx) " > - "flushing %s tid %llu, requeue\n", > + msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false); > + if (!msg) { > + pr_err("error allocating cap msg: ino (%llx.%llx) " > + "flushing %s tid %llu, requeuing cap.\n", Don't break new user-visible strings. This makes grepping harder than it should be. Thanks, Ilya
On Wed, 2020-10-07 at 19:33 +0200, Ilya Dryomov wrote: > On Wed, Oct 7, 2020 at 2:25 PM Jeff Layton <jlayton@kernel.org> wrote: > > Push the allocation of the msg and the send into the caller. Rename > > the function to marshal_cap_msg and make it void return. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ceph/caps.c | 61 +++++++++++++++++++++++++------------------------- > > 1 file changed, 30 insertions(+), 31 deletions(-) > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c > > index 4e84b39a6ebd..6f4adfaf761f 100644 > > --- a/fs/ceph/caps.c > > +++ b/fs/ceph/caps.c > > @@ -1222,36 +1222,29 @@ struct cap_msg_args { > > }; > > > > /* > > - * Build and send a cap message to the given MDS. > > - * > > - * Caller should be holding s_mutex. > > + * cap struct size + flock buffer size + inline version + inline data size + > > + * osd_epoch_barrier + oldest_flush_tid > > */ > > -static int send_cap_msg(struct cap_msg_args *arg) > > +#define CAP_MSG_SIZE (sizeof(struct ceph_mds_caps) + \ > > + 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4) > > + > > +/* Marshal up the cap msg to the MDS */ > > +static void marshal_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg) > > Nit: functions like this usually have "encode" or "build" in their > names across the codebase, so I'd go with "encode_cap_msg". > > > > { > > struct ceph_mds_caps *fc; > > - struct ceph_msg *msg; > > void *p; > > - size_t extra_len; > > struct ceph_osd_client *osdc = &arg->session->s_mdsc->fsc->client->osdc; > > > > - dout("send_cap_msg %s %llx %llx caps %s wanted %s dirty %s" > > + dout("%s %s %llx %llx caps %s wanted %s dirty %s" > > " seq %u/%u tid %llu/%llu mseq %u follows %lld size %llu/%llu" > > - " xattr_ver %llu xattr_len %d\n", ceph_cap_op_name(arg->op), > > - arg->cid, arg->ino, ceph_cap_string(arg->caps), > > - ceph_cap_string(arg->wanted), ceph_cap_string(arg->dirty), > > - arg->seq, arg->issue_seq, arg->flush_tid, arg->oldest_flush_tid, > > - arg->mseq, arg->follows, arg->size, arg->max_size, > > - arg->xattr_version, > > + " xattr_ver %llu xattr_len %d\n", __func__, > > + ceph_cap_op_name(arg->op), arg->cid, arg->ino, > > + ceph_cap_string(arg->caps), ceph_cap_string(arg->wanted), > > + ceph_cap_string(arg->dirty), arg->seq, arg->issue_seq, > > + arg->flush_tid, arg->oldest_flush_tid, arg->mseq, arg->follows, > > + arg->size, arg->max_size, arg->xattr_version, > > arg->xattr_buf ? (int)arg->xattr_buf->vec.iov_len : 0); > > > > - /* flock buffer size + inline version + inline data size + > > - * osd_epoch_barrier + oldest_flush_tid */ > > - extra_len = 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4; > > - msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, sizeof(*fc) + extra_len, > > - GFP_NOFS, false); > > - if (!msg) > > - return -ENOMEM; > > - > > msg->hdr.version = cpu_to_le16(10); > > msg->hdr.tid = cpu_to_le64(arg->flush_tid); > > > > @@ -1323,9 +1316,6 @@ static int send_cap_msg(struct cap_msg_args *arg) > > > > /* Advisory flags (version 10) */ > > ceph_encode_32(&p, arg->flags); > > - > > - ceph_con_send(&arg->session->s_con, msg); > > - return 0; > > } > > > > /* > > @@ -1456,22 +1446,24 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap, > > */ > > static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci) > > { > > + struct ceph_msg *msg; > > struct inode *inode = &ci->vfs_inode; > > - int ret; > > > > - ret = send_cap_msg(arg); > > - if (ret < 0) { > > - pr_err("error sending cap msg, ino (%llx.%llx) " > > - "flushing %s tid %llu, requeue\n", > > + msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false); > > + if (!msg) { > > + pr_err("error allocating cap msg: ino (%llx.%llx) " > > + "flushing %s tid %llu, requeuing cap.\n", > > Don't break new user-visible strings. This makes grepping harder than > it should be. > > Thanks, > > Ilya Thanks Ilya, I fixed both issues and pushed the result into testing branch. I won't bother re-posting unless there are other changes that I need to make. Cheers, -- Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 4e84b39a6ebd..6f4adfaf761f 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -1222,36 +1222,29 @@ struct cap_msg_args { }; /* - * Build and send a cap message to the given MDS. - * - * Caller should be holding s_mutex. + * cap struct size + flock buffer size + inline version + inline data size + + * osd_epoch_barrier + oldest_flush_tid */ -static int send_cap_msg(struct cap_msg_args *arg) +#define CAP_MSG_SIZE (sizeof(struct ceph_mds_caps) + \ + 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4) + +/* Marshal up the cap msg to the MDS */ +static void marshal_cap_msg(struct ceph_msg *msg, struct cap_msg_args *arg) { struct ceph_mds_caps *fc; - struct ceph_msg *msg; void *p; - size_t extra_len; struct ceph_osd_client *osdc = &arg->session->s_mdsc->fsc->client->osdc; - dout("send_cap_msg %s %llx %llx caps %s wanted %s dirty %s" + dout("%s %s %llx %llx caps %s wanted %s dirty %s" " seq %u/%u tid %llu/%llu mseq %u follows %lld size %llu/%llu" - " xattr_ver %llu xattr_len %d\n", ceph_cap_op_name(arg->op), - arg->cid, arg->ino, ceph_cap_string(arg->caps), - ceph_cap_string(arg->wanted), ceph_cap_string(arg->dirty), - arg->seq, arg->issue_seq, arg->flush_tid, arg->oldest_flush_tid, - arg->mseq, arg->follows, arg->size, arg->max_size, - arg->xattr_version, + " xattr_ver %llu xattr_len %d\n", __func__, + ceph_cap_op_name(arg->op), arg->cid, arg->ino, + ceph_cap_string(arg->caps), ceph_cap_string(arg->wanted), + ceph_cap_string(arg->dirty), arg->seq, arg->issue_seq, + arg->flush_tid, arg->oldest_flush_tid, arg->mseq, arg->follows, + arg->size, arg->max_size, arg->xattr_version, arg->xattr_buf ? (int)arg->xattr_buf->vec.iov_len : 0); - /* flock buffer size + inline version + inline data size + - * osd_epoch_barrier + oldest_flush_tid */ - extra_len = 4 + 8 + 4 + 4 + 8 + 4 + 4 + 4 + 8 + 8 + 4; - msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, sizeof(*fc) + extra_len, - GFP_NOFS, false); - if (!msg) - return -ENOMEM; - msg->hdr.version = cpu_to_le16(10); msg->hdr.tid = cpu_to_le64(arg->flush_tid); @@ -1323,9 +1316,6 @@ static int send_cap_msg(struct cap_msg_args *arg) /* Advisory flags (version 10) */ ceph_encode_32(&p, arg->flags); - - ceph_con_send(&arg->session->s_con, msg); - return 0; } /* @@ -1456,22 +1446,24 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap, */ static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci) { + struct ceph_msg *msg; struct inode *inode = &ci->vfs_inode; - int ret; - ret = send_cap_msg(arg); - if (ret < 0) { - pr_err("error sending cap msg, ino (%llx.%llx) " - "flushing %s tid %llu, requeue\n", + msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false); + if (!msg) { + pr_err("error allocating cap msg: ino (%llx.%llx) " + "flushing %s tid %llu, requeuing cap.\n", ceph_vinop(inode), ceph_cap_string(arg->dirty), arg->flush_tid); spin_lock(&ci->i_ceph_lock); __cap_delay_requeue(arg->session->s_mdsc, ci); spin_unlock(&ci->i_ceph_lock); + return; } + marshal_cap_msg(msg, arg); + ceph_con_send(&arg->session->s_con, msg); ceph_buffer_put(arg->old_xattr_buf); - if (arg->wake) wake_up_all(&ci->i_cap_wq); } @@ -1482,6 +1474,11 @@ static inline int __send_flush_snap(struct inode *inode, u32 mseq, u64 oldest_flush_tid) { struct cap_msg_args arg; + struct ceph_msg *msg; + + msg = ceph_msg_new(CEPH_MSG_CLIENT_CAPS, CAP_MSG_SIZE, GFP_NOFS, false); + if (!msg) + return -ENOMEM; arg.session = session; arg.ino = ceph_vino(inode).ino; @@ -1520,7 +1517,9 @@ static inline int __send_flush_snap(struct inode *inode, arg.flags = 0; arg.wake = false; - return send_cap_msg(&arg); + marshal_cap_msg(msg, &arg); + ceph_con_send(&arg.session->s_con, msg); + return 0; } /*
Push the allocation of the msg and the send into the caller. Rename the function to marshal_cap_msg and make it void return. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/caps.c | 61 +++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 31 deletions(-)