Re: [PATCH] libceph: clean up ceph_osdc_start_request prototype

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 3, 2022 at 3:03 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Wed, 2022-08-03 at 13:58 +0200, Ilya Dryomov wrote:
> > On Wed, Aug 3, 2022 at 1:15 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 2022-08-03 at 09:13 +0200, Ilya Dryomov wrote:
> > > > On Thu, Jun 30, 2022 at 10:21 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > > >
> > > > > This function always returns 0, and ignores the nofail boolean. Drop the
> > > > > nofail argument, make the function void return and fix up the callers.
> > > > >
> > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > > ---
> > > > >  drivers/block/rbd.c             |  6 +++---
> > > > >  fs/ceph/addr.c                  | 32 ++++++++++++--------------------
> > > > >  fs/ceph/file.c                  | 32 +++++++++++++-------------------
> > > > >  include/linux/ceph/osd_client.h |  5 ++---
> > > > >  net/ceph/osd_client.c           | 15 ++++++---------
> > > > >  5 files changed, 36 insertions(+), 54 deletions(-)
> > > > >
> > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > > > > index 91e541aa1f64..a8af0329ab77 100644
> > > > > --- a/drivers/block/rbd.c
> > > > > +++ b/drivers/block/rbd.c
> > > > > @@ -1297,7 +1297,7 @@ static void rbd_osd_submit(struct ceph_osd_request *osd_req)
> > > > >         dout("%s osd_req %p for obj_req %p objno %llu %llu~%llu\n",
> > > > >              __func__, osd_req, obj_req, obj_req->ex.oe_objno,
> > > > >              obj_req->ex.oe_off, obj_req->ex.oe_len);
> > > > > -       ceph_osdc_start_request(osd_req->r_osdc, osd_req, false);
> > > > > +       ceph_osdc_start_request(osd_req->r_osdc, osd_req);
> > > > >  }
> > > > >
> > > > >  /*
> > > > > @@ -2081,7 +2081,7 @@ static int rbd_object_map_update(struct rbd_obj_request *obj_req, u64 snap_id,
> > > > >         if (ret)
> > > > >                 return ret;
> > > > >
> > > > > -       ceph_osdc_start_request(osdc, req, false);
> > > > > +       ceph_osdc_start_request(osdc, req);
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > @@ -4768,7 +4768,7 @@ static int rbd_obj_read_sync(struct rbd_device *rbd_dev,
> > > > >         if (ret)
> > > > >                 goto out_req;
> > > > >
> > > > > -       ceph_osdc_start_request(osdc, req, false);
> > > > > +       ceph_osdc_start_request(osdc, req);
> > > > >         ret = ceph_osdc_wait_request(osdc, req);
> > > > >         if (ret >= 0)
> > > > >                 ceph_copy_from_page_vector(pages, buf, 0, ret);
> > > > > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > > > > index fe6147f20dee..66dc7844fcc6 100644
> > > > > --- a/fs/ceph/addr.c
> > > > > +++ b/fs/ceph/addr.c
> > > > > @@ -357,9 +357,7 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
> > > > >         req->r_inode = inode;
> > > > >         ihold(inode);
> > > > >
> > > > > -       err = ceph_osdc_start_request(req->r_osdc, req, false);
> > > > > -       if (err)
> > > > > -               iput(inode);
> > > > > +       ceph_osdc_start_request(req->r_osdc, req);
> > > > >  out:
> > > > >         ceph_osdc_put_request(req);
> > > > >         if (err)
> > > >
> > > > Hi Jeff,
> > > >
> > > > I'm confused by this err != 0 check.  Previously err was set to 0
> > > > by ceph_osdc_start_request() and netfs_subreq_terminated() was never
> > > > called after an OSD request submission.  Now it is called, but only if
> > > > len != 0?
> > > >
> > > > I see that netfs_subreq_terminated() accepts either the amount of data
> > > > transferred or an error code but it also has some transferred_or_error
> > > > == 0 handling which this check effectively disables.  And do we really
> > > > want to account for transferred data before the transfer occurs?
> > > >
> > >
> > > No we don't. I think you're correct. What I'm not sure of is why this
> > > doesn't cause test failures all over the place.
> >
> > This is due to "libceph: add new iov_iter-based ceph_msg_data_type and
> > ceph_osd_data_type" and "ceph: use osd_req_op_extent_osd_iter for netfs
> > reads" end up fixing it later by removing iov_iter_get_pages_alloc()
> > call.  I think because these commits were backed out of testing at one
> > point and re-added later, the order got messed up by accident.
> >
>
> Yep, that's the reason. Would you mind folding the one-line fix into the
> original patch, or would you rather I send a v2?

I already did as it seemed obviously correct to me.  I also made
sure that the fixed up patch would run through fs suite without any
follow up patches, just in case.

Thanks for jumping in on this!

                Ilya



[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux