ceph-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ceph: break up send_cap_msg
@ 2020-10-07 12:25 Jeff Layton
  2020-10-07 17:33 ` Ilya Dryomov
  0 siblings, 1 reply; 3+ messages in thread
From: Jeff Layton @ 2020-10-07 12:25 UTC (permalink / raw)
  To: ceph-devel; +Cc: idryomov, ukernel

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)
 {
 	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;
 }
 
 /*
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: break up send_cap_msg
  2020-10-07 12:25 [PATCH] ceph: break up send_cap_msg Jeff Layton
@ 2020-10-07 17:33 ` Ilya Dryomov
  2020-10-07 17:55   ` Jeff Layton
  0 siblings, 1 reply; 3+ messages in thread
From: Ilya Dryomov @ 2020-10-07 17:33 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Ceph Development, Yan, Zheng

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: break up send_cap_msg
  2020-10-07 17:33 ` Ilya Dryomov
@ 2020-10-07 17:55   ` Jeff Layton
  0 siblings, 0 replies; 3+ messages in thread
From: Jeff Layton @ 2020-10-07 17:55 UTC (permalink / raw)
  To: Ilya Dryomov; +Cc: Ceph Development, Yan, Zheng

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>


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-07 17:55 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 12:25 [PATCH] ceph: break up send_cap_msg Jeff Layton
2020-10-07 17:33 ` Ilya Dryomov
2020-10-07 17:55   ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).