* [PATCH] fix librados aio read buffer handling
@ 2013-09-30 10:13 Rutger ter Borg
2013-09-30 15:38 ` Sage Weil
0 siblings, 1 reply; 6+ messages in thread
From: Rutger ter Borg @ 2013-09-30 10:13 UTC (permalink / raw)
To: ceph-devel
[-- 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));
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix librados aio read buffer handling
2013-09-30 10:13 [PATCH] fix librados aio read buffer handling Rutger ter Borg
@ 2013-09-30 15:38 ` Sage Weil
2013-09-30 22:52 ` Josh Durgin
0 siblings, 1 reply; 6+ messages in thread
From: Sage Weil @ 2013-09-30 15:38 UTC (permalink / raw)
To: Rutger ter Borg, josh.durgin; +Cc: ceph-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix librados aio read buffer handling
2013-09-30 15:38 ` Sage Weil
@ 2013-09-30 22:52 ` Josh Durgin
2013-09-30 23:14 ` Sage Weil
2013-11-27 8:49 ` Rutger ter Borg
0 siblings, 2 replies; 6+ messages in thread
From: Josh Durgin @ 2013-09-30 22:52 UTC (permalink / raw)
To: Sage Weil, Rutger ter Borg; +Cc: ceph-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix librados aio read buffer handling
2013-09-30 22:52 ` Josh Durgin
@ 2013-09-30 23:14 ` Sage Weil
2013-11-27 8:49 ` Rutger ter Borg
1 sibling, 0 replies; 6+ messages in thread
From: Sage Weil @ 2013-09-30 23:14 UTC (permalink / raw)
To: Josh Durgin; +Cc: Rutger ter Borg, ceph-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fix librados aio read buffer handling
2013-09-30 22:52 ` Josh Durgin
2013-09-30 23:14 ` Sage Weil
@ 2013-11-27 8:49 ` Rutger ter Borg
2013-12-30 20:00 ` Josh Durgin
1 sibling, 1 reply; 6+ messages in thread
From: Rutger ter Borg @ 2013-11-27 8:49 UTC (permalink / raw)
To: ceph-devel
[-- 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));
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fix librados aio read buffer handling
2013-11-27 8:49 ` Rutger ter Borg
@ 2013-12-30 20:00 ` Josh Durgin
0 siblings, 0 replies; 6+ messages in thread
From: Josh Durgin @ 2013-12-30 20:00 UTC (permalink / raw)
To: Rutger ter Borg, ceph-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-30 20:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-30 10:13 [PATCH] fix librados aio read buffer handling Rutger ter Borg
2013-09-30 15:38 ` Sage Weil
2013-09-30 22:52 ` Josh Durgin
2013-09-30 23:14 ` Sage Weil
2013-11-27 8:49 ` Rutger ter Borg
2013-12-30 20:00 ` Josh Durgin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.