[RFC PATCH v3] ceph: add new obj copy OSD Op

[RFC PATCH v3] ceph: add new obj copy OSD Op

[Luis Henriques]

Hi,

Before going ahead with a pull-request for ceph I would like to make sure
we're all on the same page regarding the final fix for this problem.
Thus, following this email, I'm sending 2 patches: one for ceph OSDs and
the another for the kernel client.

* osd: add new 'copy-from-notrunc' operation
  This patch shall be applied to ceph master after reverting commit
  ba152435fd85 ("osd: add flag to prevent truncate_seq copy in copy-from
  operation").  It adds a new operation that will be exactly the same as
  the original 'copy-from' operation, but with the extra 2 parameters
  (truncate_{seq,size})

* ceph: switch copy_file_range to 'copy-from-notrunc' operation
  This will make the kernel client use the new OSD op in
  copy_file_range.  One extra thing that could probably be added is
  changing the mount options to NOCOPYFROM if the first call to
  ceph_osdc_copy_from() fails.

Does this look good, or did I missed something from the previous
discussion?

(One advantage of this approach: the OSD patch can be easily backported!)

Cheers,
-- 
Luis

Luis Henriques (1):
  osd: add new 'copy-from-notrunc' operation

 src/include/rados.h     |  1 +
 src/osd/OSD.cc          |  3 ++-
 src/osd/PrimaryLogPG.cc | 24 +++++++++++++++++++-----
 3 files changed, 22 insertions(+), 6 deletions(-)

  ceph: switch copy_file_range to 'copy-from-notrunc' operation

 fs/ceph/file.c                  |  3 ++-
 include/linux/ceph/osd_client.h |  1 +
 include/linux/ceph/rados.h      |  1 +
 net/ceph/osd_client.c           | 18 ++++++++++++------
 4 files changed, 16 insertions(+), 7 deletions(-)

[RFC PATCH] osd: add new 'copy-from-notrunc' operation

[Luis Henriques]

The new 'copy-from-notrunc' is very similar to the 'copy-from' operation,
except that it receives 2 extra parameters: truncate_seq and
truncate_size.

Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
 src/include/rados.h     |  1 +
 src/osd/OSD.cc          |  3 ++-
 src/osd/PrimaryLogPG.cc | 24 +++++++++++++++++++-----
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/src/include/rados.h b/src/include/rados.h
index 44cbacffcad8..bba0168c5f07 100644
--- a/src/include/rados.h
+++ b/src/include/rados.h
@@ -297,6 +297,7 @@ extern const char *ceph_osd_state_name(int s);
 									    \
 	/* tiering */							    \
 	f(COPY_FROM,	__CEPH_OSD_OP(WR, DATA, 26),	"copy-from")	    \
+	f(COPY_FROM_NOTRUNC, __CEPH_OSD_OP(WR, DATA, 45), "copy-from-notrunc") \
 	/* was copy-get-classic */					\
 	f(UNDIRTY,	__CEPH_OSD_OP(WR, DATA, 28),	"undirty")	    \
 	f(ISDIRTY,	__CEPH_OSD_OP(RD, DATA, 29),	"isdirty")	    \
diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 6b592ff85bfa..1d34bc683562 100644
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -9919,7 +9919,8 @@ int OSD::init_op_flags(OpRequestRef& op)
             (iter->op.op != CEPH_OSD_OP_RMXATTR) &&
             (iter->op.op != CEPH_OSD_OP_STARTSYNC) &&
             (iter->op.op != CEPH_OSD_OP_COPY_GET) &&
-            (iter->op.op != CEPH_OSD_OP_COPY_FROM)) {
+            (iter->op.op != CEPH_OSD_OP_COPY_FROM) &&
+	    (iter->op.op != CEPH_OSD_OP_COPY_FROM_NOTRUNC)) {
           op->set_promote();
         }
       }
diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
index 2ef4ebf8b6cf..e4f884476fc0 100644
--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -282,9 +282,11 @@ public:
   PrimaryLogPG::CopyResults *results = nullptr;
   PrimaryLogPG::OpContext *ctx;
   OSDOp &osd_op;
+  uint32_t truncate_seq;
+  uint64_t truncate_size;
 
-  CopyFromCallback(PrimaryLogPG::OpContext *ctx, OSDOp &osd_op)
-    : ctx(ctx), osd_op(osd_op) {
+  CopyFromCallback(PrimaryLogPG::OpContext *ctx, OSDOp &osd_op, uint32_t seq, uint64_t size)
+    : ctx(ctx), osd_op(osd_op), truncate_seq(seq), truncate_size(size) {
   }
   ~CopyFromCallback() override {}
 
@@ -292,6 +294,10 @@ public:
     results = results_.get<1>();
     int r = results_.get<0>();
 
+    if (osd_op.op.op != CEPH_OSD_OP_COPY_FROM_NOTRUNC) {
+      truncate_seq = results->truncate_seq;
+      truncate_size = results->truncate_size;
+    }
     // for finish_copyfrom
     ctx->user_at_version = results->user_version;
 
@@ -5668,6 +5674,7 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
     case CEPH_OSD_OP_CACHE_TRY_FLUSH:
     case CEPH_OSD_OP_UNDIRTY:
     case CEPH_OSD_OP_COPY_FROM:  // we handle user_version update explicitly
+    case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
     case CEPH_OSD_OP_CACHE_PIN:
     case CEPH_OSD_OP_CACHE_UNPIN:
     case CEPH_OSD_OP_SET_REDIRECT:
@@ -7664,17 +7671,24 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
       }
       break;
 
+    case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
     case CEPH_OSD_OP_COPY_FROM:
       ++ctx->num_write;
       result = 0;
       {
 	object_t src_name;
 	object_locator_t src_oloc;
+	uint32_t truncate_seq = 0;
+	uint64_t truncate_size = 0;
 	snapid_t src_snapid = (uint64_t)op.copy_from.snapid;
 	version_t src_version = op.copy_from.src_version;
 	try {
 	  decode(src_name, bp);
 	  decode(src_oloc, bp);
+	  if (op.op == CEPH_OSD_OP_COPY_FROM_NOTRUNC) {
+	    decode(truncate_seq, bp);
+	    decode(truncate_size, bp);
+	  }
 	}
 	catch (buffer::error& e) {
 	  result = -EINVAL;
@@ -7714,7 +7728,7 @@ int PrimaryLogPG::do_osd_ops(OpContext *ctx, vector<OSDOp>& ops)
 	    result = -EINVAL;
 	    break;
 	  }
-	  CopyFromCallback *cb = new CopyFromCallback(ctx, osd_op);
+	  CopyFromCallback *cb = new CopyFromCallback(ctx, osd_op, truncate_seq, truncate_size);
           ctx->op_finishers[ctx->current_osd_subop_num].reset(
             new CopyFromFinisher(cb));
 	  start_copy(cb, ctx->obc, src, src_oloc, src_version,
@@ -9546,8 +9560,8 @@ void PrimaryLogPG::finish_copyfrom(CopyFromCallback *cb)
     obs.oi.clear_omap_digest();
   }
 
-  obs.oi.truncate_seq = cb->results->truncate_seq;
-  obs.oi.truncate_size = cb->results->truncate_size;
+  obs.oi.truncate_seq = cb->truncate_seq;
+  obs.oi.truncate_size = cb->truncate_size;
 
   obs.oi.mtime = ceph::real_clock::to_timespec(cb->results->mtime);
   ctx->mtime = utime_t();

[RFC PATCH] ceph: switch copy_file_range to 'copy-from-notrunc' operation

[Luis Henriques]

Instead of using the 'copy-from' operation, switch copy_file_range to the
new 'copy-from-notrunc' operation, which allows to send the truncate_seq
and truncate_size parameters.

Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
 fs/ceph/file.c                  |  3 ++-
 include/linux/ceph/osd_client.h |  1 +
 include/linux/ceph/rados.h      |  1 +
 net/ceph/osd_client.c           | 18 ++++++++++++------
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index d277f71abe0b..4e0d70543d8b 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -2075,7 +2075,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 			CEPH_OSD_OP_FLAG_FADVISE_NOCACHE,
 			&dst_oid, &dst_oloc,
 			CEPH_OSD_OP_FLAG_FADVISE_SEQUENTIAL |
-			CEPH_OSD_OP_FLAG_FADVISE_DONTNEED, 0);
+			CEPH_OSD_OP_FLAG_FADVISE_DONTNEED,
+			dst_ci->i_truncate_seq, dst_ci->i_truncate_size, 0);
 		if (err) {
 			dout("ceph_osdc_copy_from returned %d\n", err);
 			if (!ret)
diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h
index eaffbdddf89a..5a62dbd3f4c2 100644
--- a/include/linux/ceph/osd_client.h
+++ b/include/linux/ceph/osd_client.h
@@ -534,6 +534,7 @@ int ceph_osdc_copy_from(struct ceph_osd_client *osdc,
 			struct ceph_object_id *dst_oid,
 			struct ceph_object_locator *dst_oloc,
 			u32 dst_fadvise_flags,
+			u32 truncate_seq, u64 truncate_size,
 			u8 copy_from_flags);
 
 /* watch/notify */
diff --git a/include/linux/ceph/rados.h b/include/linux/ceph/rados.h
index 3eb0e55665b4..54bc7648fd78 100644
--- a/include/linux/ceph/rados.h
+++ b/include/linux/ceph/rados.h
@@ -256,6 +256,7 @@ extern const char *ceph_osd_state_name(int s);
 									    \
 	/* tiering */							    \
 	f(COPY_FROM,	__CEPH_OSD_OP(WR, DATA, 26),	"copy-from")	    \
+	f(COPY_FROM_NOTRUNC, __CEPH_OSD_OP(WR, DATA, 45), "copy-from-notrunc")\
 	f(COPY_GET_CLASSIC, __CEPH_OSD_OP(RD, DATA, 27), "copy-get-classic") \
 	f(UNDIRTY,	__CEPH_OSD_OP(WR, DATA, 28),	"undirty")	    \
 	f(ISDIRTY,	__CEPH_OSD_OP(RD, DATA, 29),	"isdirty")	    \
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index ba45b074a362..f43ec0f5865c 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -402,7 +402,7 @@ static void osd_req_op_data_release(struct ceph_osd_request *osd_req,
 	case CEPH_OSD_OP_LIST_WATCHERS:
 		ceph_osd_data_release(&op->list_watchers.response_data);
 		break;
-	case CEPH_OSD_OP_COPY_FROM:
+	case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
 		ceph_osd_data_release(&op->copy_from.osd_data);
 		break;
 	default:
@@ -697,7 +697,7 @@ static void get_num_data_items(struct ceph_osd_request *req,
 		case CEPH_OSD_OP_SETXATTR:
 		case CEPH_OSD_OP_CMPXATTR:
 		case CEPH_OSD_OP_NOTIFY_ACK:
-		case CEPH_OSD_OP_COPY_FROM:
+		case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
 			*num_request_data_items += 1;
 			break;
 
@@ -1029,7 +1029,7 @@ static u32 osd_req_encode_op(struct ceph_osd_op *dst,
 	case CEPH_OSD_OP_CREATE:
 	case CEPH_OSD_OP_DELETE:
 		break;
-	case CEPH_OSD_OP_COPY_FROM:
+	case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
 		dst->copy_from.snapid = cpu_to_le64(src->copy_from.snapid);
 		dst->copy_from.src_version =
 			cpu_to_le64(src->copy_from.src_version);
@@ -1966,7 +1966,7 @@ static void setup_request_data(struct ceph_osd_request *req)
 			ceph_osdc_msg_data_add(request_msg,
 					       &op->notify_ack.request_data);
 			break;
-		case CEPH_OSD_OP_COPY_FROM:
+		case CEPH_OSD_OP_COPY_FROM_NOTRUNC:
 			ceph_osdc_msg_data_add(request_msg,
 					       &op->copy_from.osd_data);
 			break;
@@ -5315,6 +5315,7 @@ static int osd_req_op_copy_from_init(struct ceph_osd_request *req,
 				     struct ceph_object_locator *src_oloc,
 				     u32 src_fadvise_flags,
 				     u32 dst_fadvise_flags,
+				     u32 truncate_seq, u64 truncate_size,
 				     u8 copy_from_flags)
 {
 	struct ceph_osd_req_op *op;
@@ -5325,7 +5326,8 @@ static int osd_req_op_copy_from_init(struct ceph_osd_request *req,
 	if (IS_ERR(pages))
 		return PTR_ERR(pages);
 
-	op = _osd_req_op_init(req, 0, CEPH_OSD_OP_COPY_FROM, dst_fadvise_flags);
+	op = _osd_req_op_init(req, 0, CEPH_OSD_OP_COPY_FROM_NOTRUNC,
+			      dst_fadvise_flags);
 	op->copy_from.snapid = src_snapid;
 	op->copy_from.src_version = src_version;
 	op->copy_from.flags = copy_from_flags;
@@ -5335,6 +5337,8 @@ static int osd_req_op_copy_from_init(struct ceph_osd_request *req,
 	end = p + PAGE_SIZE;
 	ceph_encode_string(&p, end, src_oid->name, src_oid->name_len);
 	encode_oloc(&p, end, src_oloc);
+	ceph_encode_32(&p, truncate_seq);
+	ceph_encode_64(&p, truncate_size);
 	op->indata_len = PAGE_SIZE - (end - p);
 
 	ceph_osd_data_pages_init(&op->copy_from.osd_data, pages,
@@ -5350,6 +5354,7 @@ int ceph_osdc_copy_from(struct ceph_osd_client *osdc,
 			struct ceph_object_id *dst_oid,
 			struct ceph_object_locator *dst_oloc,
 			u32 dst_fadvise_flags,
+			u32 truncate_seq, u64 truncate_size,
 			u8 copy_from_flags)
 {
 	struct ceph_osd_request *req;
@@ -5366,7 +5371,8 @@ int ceph_osdc_copy_from(struct ceph_osd_client *osdc,
 
 	ret = osd_req_op_copy_from_init(req, src_snapid, src_version, src_oid,
 					src_oloc, src_fadvise_flags,
-					dst_fadvise_flags, copy_from_flags);
+					dst_fadvise_flags, truncate_seq,
+					truncate_size, copy_from_flags);
 	if (ret)
 		goto out;
 

Re: [RFC PATCH v3] ceph: add new obj copy OSD Op

[Jeff Layton]

On Mon, 2019-11-18 at 12:09 +0000, Luis Henriques wrote:
> Hi,
> 
> Before going ahead with a pull-request for ceph I would like to make sure
> we're all on the same page regarding the final fix for this problem.
> Thus, following this email, I'm sending 2 patches: one for ceph OSDs and
> the another for the kernel client.
> 
> * osd: add new 'copy-from-notrunc' operation
>   This patch shall be applied to ceph master after reverting commit
>   ba152435fd85 ("osd: add flag to prevent truncate_seq copy in copy-from
>   operation").  It adds a new operation that will be exactly the same as
>   the original 'copy-from' operation, but with the extra 2 parameters
>   (truncate_{seq,size})
> 
> * ceph: switch copy_file_range to 'copy-from-notrunc' operation
>   This will make the kernel client use the new OSD op in
>   copy_file_range.  One extra thing that could probably be added is
>   changing the mount options to NOCOPYFROM if the first call to
>   ceph_osdc_copy_from() fails.
> 

I probably wouldn't change the mount options to be different from what
was initially specified. How about just disable copy_file_range
internally for that superblock, and then pr_notice a message that says
that copy_file_range is being autodisabled. If they mount with '-o
nocopyfrom' that will make the warning go away.

> Does this look good, or did I missed something from the previous
> discussion?
> 
> (One advantage of this approach: the OSD patch can be easily backported!)
> 

Yep, I think this looks like a _much_ simpler approach to the problem.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH v3] ceph: add new obj copy OSD Op

[Luis Henriques]

On Mon, Nov 18, 2019 at 08:12:39AM -0500, Jeff Layton wrote:
> On Mon, 2019-11-18 at 12:09 +0000, Luis Henriques wrote:
> > Hi,
> > 
> > Before going ahead with a pull-request for ceph I would like to make sure
> > we're all on the same page regarding the final fix for this problem.
> > Thus, following this email, I'm sending 2 patches: one for ceph OSDs and
> > the another for the kernel client.
> > 
> > * osd: add new 'copy-from-notrunc' operation
> >   This patch shall be applied to ceph master after reverting commit
> >   ba152435fd85 ("osd: add flag to prevent truncate_seq copy in copy-from
> >   operation").  It adds a new operation that will be exactly the same as
> >   the original 'copy-from' operation, but with the extra 2 parameters
> >   (truncate_{seq,size})
> > 
> > * ceph: switch copy_file_range to 'copy-from-notrunc' operation
> >   This will make the kernel client use the new OSD op in
> >   copy_file_range.  One extra thing that could probably be added is
> >   changing the mount options to NOCOPYFROM if the first call to
> >   ceph_osdc_copy_from() fails.
> > 
> 
> I probably wouldn't change the mount options to be different from what
> was initially specified. How about just disable copy_file_range
> internally for that superblock, and then pr_notice a message that says
> that copy_file_range is being autodisabled. If they mount with '-o
> nocopyfrom' that will make the warning go away.

Ok, that makes sense.  I'll include this in the next rev, which will
probably be sent only after the pull-request for ceph goes in (assuming
the OSD patch won't need any major rework).

> > Does this look good, or did I missed something from the previous
> > discussion?
> > 
> > (One advantage of this approach: the OSD patch can be easily backported!)
> > 
> 
> Yep, I think this looks like a _much_ simpler approach to the problem.

Agreed!

Cheers,
--
Luís

Re: [RFC PATCH v3] ceph: add new obj copy OSD Op

[Luis Henriques]

On Mon, Nov 18, 2019 at 02:05:51PM +0000, Luis Henriques wrote:
> On Mon, Nov 18, 2019 at 08:12:39AM -0500, Jeff Layton wrote:
> > On Mon, 2019-11-18 at 12:09 +0000, Luis Henriques wrote:
> > > Hi,
> > > 
> > > Before going ahead with a pull-request for ceph I would like to make sure
> > > we're all on the same page regarding the final fix for this problem.
> > > Thus, following this email, I'm sending 2 patches: one for ceph OSDs and
> > > the another for the kernel client.
> > > 
> > > * osd: add new 'copy-from-notrunc' operation
> > >   This patch shall be applied to ceph master after reverting commit
> > >   ba152435fd85 ("osd: add flag to prevent truncate_seq copy in copy-from
> > >   operation").  It adds a new operation that will be exactly the same as
> > >   the original 'copy-from' operation, but with the extra 2 parameters
> > >   (truncate_{seq,size})
> > > 
> > > * ceph: switch copy_file_range to 'copy-from-notrunc' operation
> > >   This will make the kernel client use the new OSD op in
> > >   copy_file_range.  One extra thing that could probably be added is
> > >   changing the mount options to NOCOPYFROM if the first call to
> > >   ceph_osdc_copy_from() fails.
> > > 
> > 
> > I probably wouldn't change the mount options to be different from what
> > was initially specified. How about just disable copy_file_range
> > internally for that superblock, and then pr_notice a message that says
> > that copy_file_range is being autodisabled. If they mount with '-o
> > nocopyfrom' that will make the warning go away.
> 
> Ok, that makes sense.  I'll include this in the next rev, which will
> probably be sent only after the pull-request for ceph goes in (assuming
> the OSD patch won't need any major rework).

FYI, yesterday I created the pull-request for this [1].  I thought I had
also sent an email to this thread, but I guess I didn't... so, here it
is :-) 

[1] https://github.com/ceph/ceph/pull/31728

Cheers,
--
Luís

> 
> > > Does this look good, or did I missed something from the previous
> > > discussion?
> > > 
> > > (One advantage of this approach: the OSD patch can be easily backported!)
> > > 
> > 
> > Yep, I think this looks like a _much_ simpler approach to the problem.
> 
> Agreed!
> 
> Cheers,
> --
> Luís