[PATCH] fix librados aio read buffer handling

[PATCH] fix librados aio read buffer handling

[Rutger ter Borg]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]


Dear all,

please find attached a patch that enables a user to pass user-owned 
buffers into librados' aio_read. The patch (against dumpling) removes 
the buf and pbl data members in AioCompletionImpl.

* The 'buf' argument to read() used to be passed into AioCompletionImpl, 
and the results would be copied back after reading. This is replaced 
with the creation of a static buffer of that buf.

* The pbl argument in AioCompletionImpl is removed.

The patch is tested against an application using librados. I've assumed 
that 'pbl' in

aio_read( ...., pbl, )

is allocated by the user. It may even speed things up: a buffer copy is 
prevented.

Thanks,

Rutger


[-- Attachment #2: aio_read_patch.diff --]
[-- Type: text/x-patch, Size: 2503 bytes --]

diff -u -p ceph-0.67.3/src/librados/AioCompletionImpl.h ceph-0.67.3-patched/src/librados/AioCompletionImpl.h
--- ceph-0.67.3/src/librados/AioCompletionImpl.h	2013-09-09 21:47:34.000000000 +0200
+++ ceph-0.67.3-patched/src/librados/AioCompletionImpl.h	2013-09-30 11:14:17.946802146 +0200
@@ -39,8 +39,7 @@ struct librados::AioCompletionImpl {
 
   // for read
   bool is_read;
-  bufferlist bl, *pbl;
-  char *buf;
+  bufferlist bl;
   unsigned maxlen;
 
   IoCtxImpl *io;
@@ -50,7 +49,7 @@ struct librados::AioCompletionImpl {
   AioCompletionImpl() : lock("AioCompletionImpl lock", false, false),
 			ref(1), rval(0), released(false), ack(false), safe(false),
 			callback_complete(0), callback_safe(0), callback_arg(0),
-			is_read(false), pbl(0), buf(0), maxlen(0),
+			is_read(false), maxlen(0),
 			io(NULL), aio_write_seq(0), aio_write_list_item(this) { }
 
   int set_complete_callback(void *cb_arg, rados_callback_t cb) {
diff -u -p ceph-0.67.3/src/librados/IoCtxImpl.cc ceph-0.67.3-patched/src/librados/IoCtxImpl.cc
--- ceph-0.67.3/src/librados/IoCtxImpl.cc	2013-09-09 21:47:34.000000000 +0200
+++ ceph-0.67.3-patched/src/librados/IoCtxImpl.cc	2013-09-30 11:14:23.622824966 +0200
@@ -570,7 +570,6 @@ int librados::IoCtxImpl::aio_operate_rea
 
   c->is_read = true;
   c->io = this;
-  c->pbl = pbl;
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
@@ -613,11 +612,10 @@ int librados::IoCtxImpl::aio_read(const
 
   c->is_read = true;
   c->io = this;
-  c->pbl = pbl;
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
-		 off, len, snapid, &c->bl, 0,
+		 off, len, snapid, pbl, 0,
 		 onack, &c->objver);
   return 0;
 }
@@ -633,8 +631,9 @@ int librados::IoCtxImpl::aio_read(const
 
   c->is_read = true;
   c->io = this;
-  c->buf = buf;
   c->maxlen = len;
+  c->bl.clear();
+  c->bl.push_back( buffer::create_static( len, buf ) );
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
@@ -669,7 +668,6 @@ int librados::IoCtxImpl::aio_sparse_read
 
   c->is_read = true;
   c->io = this;
-  c->pbl = NULL;
 
   onack->m_ops.sparse_read(off, len, m, data_bl, NULL);
 
@@ -1180,15 +1178,6 @@ void librados::IoCtxImpl::C_aio_Ack::fin
     c->safe = true;
   c->cond.Signal();
 
-  if (c->buf && c->bl.length() > 0) {
-    unsigned l = MIN(c->bl.length(), c->maxlen);
-    c->bl.copy(0, l, c->buf);
-    c->rval = c->bl.length();
-  }
-  if (c->pbl) {
-    *c->pbl = c->bl;
-  }
-
   if (c->callback_complete) {
     c->io->client->finisher.queue(new C_AioComplete(c));
   }

Re: [PATCH] fix librados aio read buffer handling

[Sage Weil]

On Mon, 30 Sep 2013, Rutger ter Borg wrote:
> 
> Dear all,
> 
> please find attached a patch that enables a user to pass user-owned buffers
> into librados' aio_read. The patch (against dumpling) removes the buf and pbl
> data members in AioCompletionImpl.
> 
> * The 'buf' argument to read() used to be passed into AioCompletionImpl, and
> the results would be copied back after reading. This is replaced with the
> creation of a static buffer of that buf.
> 
> * The pbl argument in AioCompletionImpl is removed.
> 
> The patch is tested against an application using librados. I've assumed that
> 'pbl' in
> 
> aio_read( ...., pbl, )
> 
> is allocated by the user. It may even speed things up: a buffer copy is
> prevented.

I am a little worried that one path of aio_read uses c->bl and the other 
doesn't, but that probably is no big deal provided it is noted in the 
structure definition.

My larger concern is that we're about to do some major changes in the 
messenger and other code to use splice/tee/vmsplice to avoid copies 
to/from userspace when possible.  That will involve removing some of the 
currently 'use the existing buffer' code.  I'm hoping it will work out 
that in the librados case we just carry the kernel pages around a bit 
longer and delay the final copy into userspace, but it's hard to say until 
the code gets written.  Josh plans to start working on it this week.

Josh, do you think we should apply this now or wait until we see where 
things end up?

sage

Re: [PATCH] fix librados aio read buffer handling

[Josh Durgin]

On 09/30/2013 08:38 AM, Sage Weil wrote:
> On Mon, 30 Sep 2013, Rutger ter Borg wrote:
>>
>> Dear all,
>>
>> please find attached a patch that enables a user to pass user-owned buffers
>> into librados' aio_read. The patch (against dumpling) removes the buf and pbl
>> data members in AioCompletionImpl.
>>
>> * The 'buf' argument to read() used to be passed into AioCompletionImpl, and
>> the results would be copied back after reading. This is replaced with the
>> creation of a static buffer of that buf.
>>
>> * The pbl argument in AioCompletionImpl is removed.
>>
>> The patch is tested against an application using librados. I've assumed that
>> 'pbl' in
>>
>> aio_read( ...., pbl, )
>>
>> is allocated by the user. It may even speed things up: a buffer copy is
>> prevented.
>
> I am a little worried that one path of aio_read uses c->bl and the other
> doesn't, but that probably is no big deal provided it is noted in the
> structure definition.

It does clean up the existing usage, where the destination may be
c->buf or c->pbl though.

> My larger concern is that we're about to do some major changes in the
> messenger and other code to use splice/tee/vmsplice to avoid copies
> to/from userspace when possible.  That will involve removing some of the
> currently 'use the existing buffer' code.  I'm hoping it will work out
> that in the librados case we just carry the kernel pages around a bit
> longer and delay the final copy into userspace, but it's hard to say until
> the code gets written.  Josh plans to start working on it this week.
>
> Josh, do you think we should apply this now or wait until we see where
> things end up?

I'm fine applying this now (with one fix). It's a nice cleanup
even if things change more soon.

For the C interface, the return value stored in the AioCompletionImpl
needs to be the length read, so the caller can tell if a short read
occurred (this is only possible when trying to read past the end of an
object). This was being set in C_aio_Ack::finish(), but was removed by
this patch.

One thing I'm not sure about is whether the bufferlist is guaranteed
not to be split anywhere in the lower levels. rados_read()
accounts for this case:

     if (bl.c_str() != buf)
       bl.copy(0, bl.length(), buf);

Sage, is that actually necessary?

Josh

Re: [PATCH] fix librados aio read buffer handling

[Sage Weil]

On Mon, 30 Sep 2013, Josh Durgin wrote:
> On 09/30/2013 08:38 AM, Sage Weil wrote:
> > On Mon, 30 Sep 2013, Rutger ter Borg wrote:
> > > 
> > > Dear all,
> > > 
> > > please find attached a patch that enables a user to pass user-owned
> > > buffers
> > > into librados' aio_read. The patch (against dumpling) removes the buf and
> > > pbl
> > > data members in AioCompletionImpl.
> > > 
> > > * The 'buf' argument to read() used to be passed into AioCompletionImpl,
> > > and
> > > the results would be copied back after reading. This is replaced with the
> > > creation of a static buffer of that buf.
> > > 
> > > * The pbl argument in AioCompletionImpl is removed.
> > > 
> > > The patch is tested against an application using librados. I've assumed
> > > that
> > > 'pbl' in
> > > 
> > > aio_read( ...., pbl, )
> > > 
> > > is allocated by the user. It may even speed things up: a buffer copy is
> > > prevented.
> > 
> > I am a little worried that one path of aio_read uses c->bl and the other
> > doesn't, but that probably is no big deal provided it is noted in the
> > structure definition.
> 
> It does clean up the existing usage, where the destination may be
> c->buf or c->pbl though.
> 
> > My larger concern is that we're about to do some major changes in the
> > messenger and other code to use splice/tee/vmsplice to avoid copies
> > to/from userspace when possible.  That will involve removing some of the
> > currently 'use the existing buffer' code.  I'm hoping it will work out
> > that in the librados case we just carry the kernel pages around a bit
> > longer and delay the final copy into userspace, but it's hard to say until
> > the code gets written.  Josh plans to start working on it this week.
> > 
> > Josh, do you think we should apply this now or wait until we see where
> > things end up?
> 
> I'm fine applying this now (with one fix). It's a nice cleanup
> even if things change more soon.
> 
> For the C interface, the return value stored in the AioCompletionImpl
> needs to be the length read, so the caller can tell if a short read
> occurred (this is only possible when trying to read past the end of an
> object). This was being set in C_aio_Ack::finish(), but was removed by
> this patch.
> 
> One thing I'm not sure about is whether the bufferlist is guaranteed
> not to be split anywhere in the lower levels. rados_read()
> accounts for this case:
> 
>     if (bl.c_str() != buf)
>       bl.copy(0, bl.length(), buf);
> 
> Sage, is that actually necessary?

I think it's worthwhile as a safety check.  Even if is not currently 
necessary now the implementation could change such that it is later.

sage

Re: [PATCH] fix librados aio read buffer handling

[Rutger ter Borg]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On 2013-10-01 00:52, Josh Durgin wrote:
>
> I'm fine applying this now (with one fix). It's a nice cleanup
> even if things change more soon.
>
> For the C interface, the return value stored in the AioCompletionImpl
> needs to be the length read, so the caller can tell if a short read
> occurred (this is only possible when trying to read past the end of an
> object). This was being set in C_aio_Ack::finish(), but was removed by
> this patch.

Please find attached a revised patch against 0.72.1. I've added a fix 
for setting the return value in C_aio_Ack::finish(),

if (c->bl.length() > 0) {
   c->rval = c->bl.length();
}

Cheers,

Rutger



[-- Attachment #2: aio_read_patch_2.diff --]
[-- Type: text/x-patch, Size: 2410 bytes --]

diff -u -p ceph-0.72.1/src/librados/AioCompletionImpl.h ceph-0.72.1-patched/src/librados/AioCompletionImpl.h
--- ceph-0.72.1/src/librados/AioCompletionImpl.h	2013-11-22 10:15:58.000000000 +0100
+++ ceph-0.72.1-patched/src/librados/AioCompletionImpl.h	2013-11-25 11:32:06.178467454 +0100
@@ -39,8 +39,7 @@ struct librados::AioCompletionImpl {
 
   // for read
   bool is_read;
-  bufferlist bl, *pbl;
-  char *buf;
+  bufferlist bl;
   unsigned maxlen;
 
   IoCtxImpl *io;
@@ -54,7 +53,7 @@ struct librados::AioCompletionImpl {
 			callback_safe(0),
 			callback_complete_arg(0),
 			callback_safe_arg(0),
-			is_read(false), pbl(0), buf(0), maxlen(0),
+			is_read(false), maxlen(0),
 			io(NULL), aio_write_seq(0), aio_write_list_item(this) { }
 
   int set_complete_callback(void *cb_arg, rados_callback_t cb) {
diff -u -p ceph-0.72.1/src/librados/IoCtxImpl.cc ceph-0.72.1-patched/src/librados/IoCtxImpl.cc
--- ceph-0.72.1/src/librados/IoCtxImpl.cc	2013-11-22 10:15:58.000000000 +0100
+++ ceph-0.72.1-patched/src/librados/IoCtxImpl.cc	2013-11-25 11:40:41.952777192 +0100
@@ -570,7 +570,6 @@ int librados::IoCtxImpl::aio_operate_rea
 
   c->is_read = true;
   c->io = this;
-  c->pbl = pbl;
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
@@ -612,11 +611,10 @@ int librados::IoCtxImpl::aio_read(const
 
   c->is_read = true;
   c->io = this;
-  c->pbl = pbl;
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
-		 off, len, snapid, &c->bl, 0,
+		 off, len, snapid, pbl, 0,
 		 onack, &c->objver);
   return 0;
 }
@@ -632,8 +630,9 @@ int librados::IoCtxImpl::aio_read(const
 
   c->is_read = true;
   c->io = this;
-  c->buf = buf;
   c->maxlen = len;
+  c->bl.clear();
+  c->bl.push_back( buffer::create_static( len, buf ) );
 
   Mutex::Locker l(*lock);
   objecter->read(oid, oloc,
@@ -668,7 +667,6 @@ int librados::IoCtxImpl::aio_sparse_read
 
   c->is_read = true;
   c->io = this;
-  c->pbl = NULL;
 
   onack->m_ops.sparse_read(off, len, m, data_bl, NULL);
 
@@ -1179,14 +1177,9 @@ void librados::IoCtxImpl::C_aio_Ack::fin
     c->safe = true;
   c->cond.Signal();
 
-  if (c->buf && c->bl.length() > 0) {
-    unsigned l = MIN(c->bl.length(), c->maxlen);
-    c->bl.copy(0, l, c->buf);
+  if (c->bl.length() > 0) {
     c->rval = c->bl.length();
   }
-  if (c->pbl) {
-    *c->pbl = c->bl;
-  }
 
   if (c->callback_complete) {
     c->io->client->finisher.queue(new C_AioComplete(c));

Re: [PATCH] fix librados aio read buffer handling

[Josh Durgin]

On 11/27/2013 12:49 AM, Rutger ter Borg wrote:
> On 2013-10-01 00:52, Josh Durgin wrote:
>>
>> I'm fine applying this now (with one fix). It's a nice cleanup
>> even if things change more soon.
>>
>> For the C interface, the return value stored in the AioCompletionImpl
>> needs to be the length read, so the caller can tell if a short read
>> occurred (this is only possible when trying to read past the end of an
>> object). This was being set in C_aio_Ack::finish(), but was removed by
>> this patch.
>
> Please find attached a revised patch against 0.72.1. I've added a fix
> for setting the return value in C_aio_Ack::finish(),
>
> if (c->bl.length() > 0) {
>    c->rval = c->bl.length();
> }

Sorry for the delay - I just added this to the master branch.

Thanks!
Josh