| Message ID | 20231106011644.248119-3-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libceph: sparse-read misc fixes | expand |
On Mon, Nov 6, 2023 at 2:19 AM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > For sparse reading the real length of the data should equal to the > total length from the extent array. > > URL: https://tracker.ceph.com/issues/62081 > Signed-off-by: Xiubo Li <xiubli@redhat.com> > Reviewed-by: Jeff Layton <jlayton@kernel.org> > --- > net/ceph/osd_client.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 0e629dfd55ee..050dc39065fb 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -5920,6 +5920,12 @@ static int osd_sparse_read(struct ceph_connection *con, > fallthrough; > case CEPH_SPARSE_READ_DATA: > if (sr->sr_index >= count) { > + if (sr->sr_datalen && count) { > + pr_warn_ratelimited("%s: datalen and extents mismath, %d left\n", > + __func__, sr->sr_datalen); > + return -EREMOTEIO; By returning EREMOTEIO here you have significantly changed the semantics (in v2 it was just a warning) but Jeff's Reviewed-by is retained. Has he acked the change? Thanks, Ilya
On 11/6/23 19:54, Ilya Dryomov wrote: > On Mon, Nov 6, 2023 at 2:19 AM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> For sparse reading the real length of the data should equal to the >> total length from the extent array. >> >> URL: https://tracker.ceph.com/issues/62081 >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> Reviewed-by: Jeff Layton <jlayton@kernel.org> >> --- >> net/ceph/osd_client.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> index 0e629dfd55ee..050dc39065fb 100644 >> --- a/net/ceph/osd_client.c >> +++ b/net/ceph/osd_client.c >> @@ -5920,6 +5920,12 @@ static int osd_sparse_read(struct ceph_connection *con, >> fallthrough; >> case CEPH_SPARSE_READ_DATA: >> if (sr->sr_index >= count) { >> + if (sr->sr_datalen && count) { >> + pr_warn_ratelimited("%s: datalen and extents mismath, %d left\n", >> + __func__, sr->sr_datalen); >> + return -EREMOTEIO; > By returning EREMOTEIO here you have significantly changed the > semantics (in v2 it was just a warning) but Jeff's Reviewed-by is > retained. Has he acked the change? Oh, sorry I forgot to remove that. Jeff, Please take a look here again. Thanks - Xiubo > Thanks, > > Ilya >
On Mon, 2023-11-06 at 20:17 +0800, Xiubo Li wrote: > On 11/6/23 19:54, Ilya Dryomov wrote: > > On Mon, Nov 6, 2023 at 2:19 AM <xiubli@redhat.com> wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > For sparse reading the real length of the data should equal to the > > > total length from the extent array. > > > > > > URL: https://tracker.ceph.com/issues/62081 > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > > Reviewed-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > net/ceph/osd_client.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > > index 0e629dfd55ee..050dc39065fb 100644 > > > --- a/net/ceph/osd_client.c > > > +++ b/net/ceph/osd_client.c > > > @@ -5920,6 +5920,12 @@ static int osd_sparse_read(struct ceph_connection *con, > > > fallthrough; > > > case CEPH_SPARSE_READ_DATA: > > > if (sr->sr_index >= count) { > > > + if (sr->sr_datalen && count) { > > > + pr_warn_ratelimited("%s: datalen and extents mismath, %d left\n", > > > + __func__, sr->sr_datalen); > > > + return -EREMOTEIO; > > By returning EREMOTEIO here you have significantly changed the > > semantics (in v2 it was just a warning) but Jeff's Reviewed-by is > > retained. Has he acked the change? > > Oh, sorry I forgot to remove that. > > Jeff, Please take a look here again. > > Thanks > > - Xiubo > > Returning EREMOTEIO if the lengths don't match seems like a reasonable thing to do. You can retain the R-b.
On 11/6/23 20:32, Jeff Layton wrote: > On Mon, 2023-11-06 at 20:17 +0800, Xiubo Li wrote: >> On 11/6/23 19:54, Ilya Dryomov wrote: >>> On Mon, Nov 6, 2023 at 2:19 AM <xiubli@redhat.com> wrote: >>>> From: Xiubo Li <xiubli@redhat.com> >>>> >>>> For sparse reading the real length of the data should equal to the >>>> total length from the extent array. >>>> >>>> URL: https://tracker.ceph.com/issues/62081 >>>> Signed-off-by: Xiubo Li <xiubli@redhat.com> >>>> Reviewed-by: Jeff Layton <jlayton@kernel.org> >>>> --- >>>> net/ceph/osd_client.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >>>> index 0e629dfd55ee..050dc39065fb 100644 >>>> --- a/net/ceph/osd_client.c >>>> +++ b/net/ceph/osd_client.c >>>> @@ -5920,6 +5920,12 @@ static int osd_sparse_read(struct ceph_connection *con, >>>> fallthrough; >>>> case CEPH_SPARSE_READ_DATA: >>>> if (sr->sr_index >= count) { >>>> + if (sr->sr_datalen && count) { >>>> + pr_warn_ratelimited("%s: datalen and extents mismath, %d left\n", >>>> + __func__, sr->sr_datalen); >>>> + return -EREMOTEIO; >>> By returning EREMOTEIO here you have significantly changed the >>> semantics (in v2 it was just a warning) but Jeff's Reviewed-by is >>> retained. Has he acked the change? >> Oh, sorry I forgot to remove that. >> >> Jeff, Please take a look here again. >> >> Thanks >> >> - Xiubo >> >> > Returning EREMOTEIO if the lengths don't match seems like a reasonable > thing to do. You can retain the R-b. > Thanks Jeff. - Xiubo
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 0e629dfd55ee..050dc39065fb 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -5920,6 +5920,12 @@ static int osd_sparse_read(struct ceph_connection *con, fallthrough; case CEPH_SPARSE_READ_DATA: if (sr->sr_index >= count) { + if (sr->sr_datalen && count) { + pr_warn_ratelimited("%s: datalen and extents mismath, %d left\n", + __func__, sr->sr_datalen); + return -EREMOTEIO; + } + sr->sr_state = CEPH_SPARSE_READ_HDR; goto next_op; } @@ -5927,6 +5933,8 @@ static int osd_sparse_read(struct ceph_connection *con, eoff = sr->sr_extent[sr->sr_index].off; elen = sr->sr_extent[sr->sr_index].len; + sr->sr_datalen -= elen; + dout("[%d] ext %d off 0x%llx len 0x%llx\n", o->o_osd, sr->sr_index, eoff, elen);