| Message ID | l2bire$iue$1@ger.gmane.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)); }