diff mbox series

[02/20] rbd: replace obj_req->tried_parent with obj_req->read_state

Message ID 20190625144111.11270-3-idryomov@gmail.com (mailing list archive)
State New, archived
Headers show
Series rbd: support for object-map and fast-diff | expand

Commit Message

Ilya Dryomov June 25, 2019, 2:40 p.m. UTC 
Make rbd_obj_handle_read() look like a state machine and get rid of
the necessity to patch result in rbd_obj_handle_request(), completing
the removal of obj_req->xferred and img_req->xferred.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 drivers/block/rbd.c | 82 +++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 36 deletions(-)

Comments

Dongsheng Yang July 1, 2019, 5:28 a.m. UTC | #1 
On 06/25/2019 10:40 PM, Ilya Dryomov wrote:
> Make rbd_obj_handle_read() look like a state machine and get rid of
> the necessity to patch result in rbd_obj_handle_request(), completing
> the removal of obj_req->xferred and img_req->xferred.
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>   drivers/block/rbd.c | 82 +++++++++++++++++++++++++--------------------
>   1 file changed, 46 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a9b0b23148f9..7925b2fdde79 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -219,6 +219,11 @@ enum obj_operation_type {
>   	OBJ_OP_ZEROOUT,
>   };
>   
> +enum rbd_obj_read_state {
> +	RBD_OBJ_READ_OBJECT = 1,
> +	RBD_OBJ_READ_PARENT,
> +};
> +
>   /*
>    * Writes go through the following state machine to deal with
>    * layering:
> @@ -255,7 +260,7 @@ enum rbd_obj_write_state {
>   struct rbd_obj_request {
>   	struct ceph_object_extent ex;
>   	union {
> -		bool			tried_parent;	/* for reads */
> +		enum rbd_obj_read_state	 read_state;	/* for reads */
>   		enum rbd_obj_write_state write_state;	/* for writes */
>   	};
>   
> @@ -1794,6 +1799,7 @@ static int rbd_obj_setup_read(struct rbd_obj_request *obj_req)
>   	rbd_osd_req_setup_data(obj_req, 0);
>   
>   	rbd_osd_req_format_read(obj_req);
> +	obj_req->read_state = RBD_OBJ_READ_OBJECT;
>   	return 0;
>   }
>   
> @@ -2402,44 +2408,48 @@ static bool rbd_obj_handle_read(struct rbd_obj_request *obj_req, int *result)
>   	struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
>   	int ret;
>   
> -	if (*result == -ENOENT &&
> -	    rbd_dev->parent_overlap && !obj_req->tried_parent) {
> -		/* reverse map this object extent onto the parent */
> -		ret = rbd_obj_calc_img_extents(obj_req, false);
> -		if (ret) {
> -			*result = ret;
> -			return true;
> -		}
> -
> -		if (obj_req->num_img_extents) {
> -			obj_req->tried_parent = true;
> -			ret = rbd_obj_read_from_parent(obj_req);
> +	switch (obj_req->read_state) {
> +	case RBD_OBJ_READ_OBJECT:
> +		if (*result == -ENOENT && rbd_dev->parent_overlap) {
> +			/* reverse map this object extent onto the parent */
> +			ret = rbd_obj_calc_img_extents(obj_req, false);
>   			if (ret) {
>   				*result = ret;
>   				return true;
>   			}
> -			return false;
> +			if (obj_req->num_img_extents) {
> +				ret = rbd_obj_read_from_parent(obj_req);
> +				if (ret) {
> +					*result = ret;
> +					return true;
> +				}
> +				obj_req->read_state = RBD_OBJ_READ_PARENT;
Seems there is a race window between the read request complete but the 
read_state is still RBD_OBJ_READ_OBJECT.
> +				return false;
> +			}
>   		}
> -	}
>   
> -	/*
> -	 * -ENOENT means a hole in the image -- zero-fill the entire
> -	 * length of the request.  A short read also implies zero-fill
> -	 * to the end of the request.
> -	 */
> -	if (*result == -ENOENT) {
> -		rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
> -		*result = 0;
> -	} else if (*result >= 0) {
> -		if (*result < obj_req->ex.oe_len)
> -			rbd_obj_zero_range(obj_req, *result,
> -					   obj_req->ex.oe_len - *result);
> -		else
> -			rbd_assert(*result == obj_req->ex.oe_len);
> -		*result = 0;
> +		/*
> +		 * -ENOENT means a hole in the image -- zero-fill the entire
> +		 * length of the request.  A short read also implies zero-fill
> +		 * to the end of the request.
> +		 */
> +		if (*result == -ENOENT) {
> +			rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
> +			*result = 0;
> +		} else if (*result >= 0) {
> +			if (*result < obj_req->ex.oe_len)
> +				rbd_obj_zero_range(obj_req, *result,
> +						obj_req->ex.oe_len - *result);
> +			else
> +				rbd_assert(*result == obj_req->ex.oe_len);
> +			*result = 0;
> +		}
> +		return true;
> +	case RBD_OBJ_READ_PARENT:
> +		return true;
> +	default:
> +		BUG();
>   	}
> -
> -	return true;
>   }
>   
>   /*
> @@ -2658,11 +2668,11 @@ static bool rbd_obj_handle_write(struct rbd_obj_request *obj_req, int *result)
>   	case RBD_OBJ_WRITE_COPYUP_OPS:
>   		return true;
>   	case RBD_OBJ_WRITE_READ_FROM_PARENT:
> -		if (*result < 0)
> +		if (*result)
>   			return true;
>   
> -		rbd_assert(*result);
> -		ret = rbd_obj_issue_copyup(obj_req, *result);
> +		ret = rbd_obj_issue_copyup(obj_req,
> +					   rbd_obj_img_extents_bytes(obj_req));
>   		if (ret) {
>   			*result = ret;
>   			return true;
> @@ -2757,7 +2767,7 @@ static void rbd_obj_handle_request(struct rbd_obj_request *obj_req, int result)
>   	rbd_assert(img_req->result <= 0);
>   	if (test_bit(IMG_REQ_CHILD, &img_req->flags)) {
>   		obj_req = img_req->obj_request;
> -		result = img_req->result ?: rbd_obj_img_extents_bytes(obj_req);
> +		result = img_req->result;
>   		rbd_img_request_put(img_req);
>   		goto again;
>   	}
should this part be in 01/20 ?
Thanx
Ilya Dryomov July 3, 2019, 8:24 a.m. UTC | #2 
On Mon, Jul 1, 2019 at 7:28 AM Dongsheng Yang
<dongsheng.yang@easystack.cn> wrote:
>
>
>
> On 06/25/2019 10:40 PM, Ilya Dryomov wrote:
> > Make rbd_obj_handle_read() look like a state machine and get rid of
> > the necessity to patch result in rbd_obj_handle_request(), completing
> > the removal of obj_req->xferred and img_req->xferred.
> >
> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > ---
> >   drivers/block/rbd.c | 82 +++++++++++++++++++++++++--------------------
> >   1 file changed, 46 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > index a9b0b23148f9..7925b2fdde79 100644
> > --- a/drivers/block/rbd.c
> > +++ b/drivers/block/rbd.c
> > @@ -219,6 +219,11 @@ enum obj_operation_type {
> >       OBJ_OP_ZEROOUT,
> >   };
> >
> > +enum rbd_obj_read_state {
> > +     RBD_OBJ_READ_OBJECT = 1,
> > +     RBD_OBJ_READ_PARENT,
> > +};
> > +
> >   /*
> >    * Writes go through the following state machine to deal with
> >    * layering:
> > @@ -255,7 +260,7 @@ enum rbd_obj_write_state {
> >   struct rbd_obj_request {
> >       struct ceph_object_extent ex;
> >       union {
> > -             bool                    tried_parent;   /* for reads */
> > +             enum rbd_obj_read_state  read_state;    /* for reads */
> >               enum rbd_obj_write_state write_state;   /* for writes */
> >       };
> >
> > @@ -1794,6 +1799,7 @@ static int rbd_obj_setup_read(struct rbd_obj_request *obj_req)
> >       rbd_osd_req_setup_data(obj_req, 0);
> >
> >       rbd_osd_req_format_read(obj_req);
> > +     obj_req->read_state = RBD_OBJ_READ_OBJECT;
> >       return 0;
> >   }
> >
> > @@ -2402,44 +2408,48 @@ static bool rbd_obj_handle_read(struct rbd_obj_request *obj_req, int *result)
> >       struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
> >       int ret;
> >
> > -     if (*result == -ENOENT &&
> > -         rbd_dev->parent_overlap && !obj_req->tried_parent) {
> > -             /* reverse map this object extent onto the parent */
> > -             ret = rbd_obj_calc_img_extents(obj_req, false);
> > -             if (ret) {
> > -                     *result = ret;
> > -                     return true;
> > -             }
> > -
> > -             if (obj_req->num_img_extents) {
> > -                     obj_req->tried_parent = true;
> > -                     ret = rbd_obj_read_from_parent(obj_req);
> > +     switch (obj_req->read_state) {
> > +     case RBD_OBJ_READ_OBJECT:
> > +             if (*result == -ENOENT && rbd_dev->parent_overlap) {
> > +                     /* reverse map this object extent onto the parent */
> > +                     ret = rbd_obj_calc_img_extents(obj_req, false);
> >                       if (ret) {
> >                               *result = ret;
> >                               return true;
> >                       }
> > -                     return false;
> > +                     if (obj_req->num_img_extents) {
> > +                             ret = rbd_obj_read_from_parent(obj_req);
> > +                             if (ret) {
> > +                                     *result = ret;
> > +                                     return true;
> > +                             }
> > +                             obj_req->read_state = RBD_OBJ_READ_PARENT;
> Seems there is a race window between the read request complete but the
> read_state is still RBD_OBJ_READ_OBJECT.

Yes, this is resolved with the addition of obj_req->state_mutex later
in the series.

> > +                             return false;
> > +                     }
> >               }
> > -     }
> >
> > -     /*
> > -      * -ENOENT means a hole in the image -- zero-fill the entire
> > -      * length of the request.  A short read also implies zero-fill
> > -      * to the end of the request.
> > -      */
> > -     if (*result == -ENOENT) {
> > -             rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
> > -             *result = 0;
> > -     } else if (*result >= 0) {
> > -             if (*result < obj_req->ex.oe_len)
> > -                     rbd_obj_zero_range(obj_req, *result,
> > -                                        obj_req->ex.oe_len - *result);
> > -             else
> > -                     rbd_assert(*result == obj_req->ex.oe_len);
> > -             *result = 0;
> > +             /*
> > +              * -ENOENT means a hole in the image -- zero-fill the entire
> > +              * length of the request.  A short read also implies zero-fill
> > +              * to the end of the request.
> > +              */
> > +             if (*result == -ENOENT) {
> > +                     rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
> > +                     *result = 0;
> > +             } else if (*result >= 0) {
> > +                     if (*result < obj_req->ex.oe_len)
> > +                             rbd_obj_zero_range(obj_req, *result,
> > +                                             obj_req->ex.oe_len - *result);
> > +                     else
> > +                             rbd_assert(*result == obj_req->ex.oe_len);
> > +                     *result = 0;
> > +             }
> > +             return true;
> > +     case RBD_OBJ_READ_PARENT:
> > +             return true;
> > +     default:
> > +             BUG();
> >       }
> > -
> > -     return true;
> >   }
> >
> >   /*
> > @@ -2658,11 +2668,11 @@ static bool rbd_obj_handle_write(struct rbd_obj_request *obj_req, int *result)
> >       case RBD_OBJ_WRITE_COPYUP_OPS:
> >               return true;
> >       case RBD_OBJ_WRITE_READ_FROM_PARENT:
> > -             if (*result < 0)
> > +             if (*result)
> >                       return true;
> >
> > -             rbd_assert(*result);
> > -             ret = rbd_obj_issue_copyup(obj_req, *result);
> > +             ret = rbd_obj_issue_copyup(obj_req,
> > +                                        rbd_obj_img_extents_bytes(obj_req));
> >               if (ret) {
> >                       *result = ret;
> >                       return true;
> > @@ -2757,7 +2767,7 @@ static void rbd_obj_handle_request(struct rbd_obj_request *obj_req, int result)
> >       rbd_assert(img_req->result <= 0);
> >       if (test_bit(IMG_REQ_CHILD, &img_req->flags)) {
> >               obj_req = img_req->obj_request;
> > -             result = img_req->result ?: rbd_obj_img_extents_bytes(obj_req);
> > +             result = img_req->result;
> >               rbd_img_request_put(img_req);
> >               goto again;
> >       }
> should this part be in 01/20 ?

No, 01/20 wouldn't pass a basic copyup test with that.

Thanks,

                Ilya
diff mbox series

Patch

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a9b0b23148f9..7925b2fdde79 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -219,6 +219,11 @@  enum obj_operation_type {
 	OBJ_OP_ZEROOUT,
 };
 
+enum rbd_obj_read_state {
+	RBD_OBJ_READ_OBJECT = 1,
+	RBD_OBJ_READ_PARENT,
+};
+
 /*
  * Writes go through the following state machine to deal with
  * layering:
@@ -255,7 +260,7 @@  enum rbd_obj_write_state {
 struct rbd_obj_request {
 	struct ceph_object_extent ex;
 	union {
-		bool			tried_parent;	/* for reads */
+		enum rbd_obj_read_state	 read_state;	/* for reads */
 		enum rbd_obj_write_state write_state;	/* for writes */
 	};
 
@@ -1794,6 +1799,7 @@  static int rbd_obj_setup_read(struct rbd_obj_request *obj_req)
 	rbd_osd_req_setup_data(obj_req, 0);
 
 	rbd_osd_req_format_read(obj_req);
+	obj_req->read_state = RBD_OBJ_READ_OBJECT;
 	return 0;
 }
 
@@ -2402,44 +2408,48 @@  static bool rbd_obj_handle_read(struct rbd_obj_request *obj_req, int *result)
 	struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
 	int ret;
 
-	if (*result == -ENOENT &&
-	    rbd_dev->parent_overlap && !obj_req->tried_parent) {
-		/* reverse map this object extent onto the parent */
-		ret = rbd_obj_calc_img_extents(obj_req, false);
-		if (ret) {
-			*result = ret;
-			return true;
-		}
-
-		if (obj_req->num_img_extents) {
-			obj_req->tried_parent = true;
-			ret = rbd_obj_read_from_parent(obj_req);
+	switch (obj_req->read_state) {
+	case RBD_OBJ_READ_OBJECT:
+		if (*result == -ENOENT && rbd_dev->parent_overlap) {
+			/* reverse map this object extent onto the parent */
+			ret = rbd_obj_calc_img_extents(obj_req, false);
 			if (ret) {
 				*result = ret;
 				return true;
 			}
-			return false;
+			if (obj_req->num_img_extents) {
+				ret = rbd_obj_read_from_parent(obj_req);
+				if (ret) {
+					*result = ret;
+					return true;
+				}
+				obj_req->read_state = RBD_OBJ_READ_PARENT;
+				return false;
+			}
 		}
-	}
 
-	/*
-	 * -ENOENT means a hole in the image -- zero-fill the entire
-	 * length of the request.  A short read also implies zero-fill
-	 * to the end of the request.
-	 */
-	if (*result == -ENOENT) {
-		rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
-		*result = 0;
-	} else if (*result >= 0) {
-		if (*result < obj_req->ex.oe_len)
-			rbd_obj_zero_range(obj_req, *result,
-					   obj_req->ex.oe_len - *result);
-		else
-			rbd_assert(*result == obj_req->ex.oe_len);
-		*result = 0;
+		/*
+		 * -ENOENT means a hole in the image -- zero-fill the entire
+		 * length of the request.  A short read also implies zero-fill
+		 * to the end of the request.
+		 */
+		if (*result == -ENOENT) {
+			rbd_obj_zero_range(obj_req, 0, obj_req->ex.oe_len);
+			*result = 0;
+		} else if (*result >= 0) {
+			if (*result < obj_req->ex.oe_len)
+				rbd_obj_zero_range(obj_req, *result,
+						obj_req->ex.oe_len - *result);
+			else
+				rbd_assert(*result == obj_req->ex.oe_len);
+			*result = 0;
+		}
+		return true;
+	case RBD_OBJ_READ_PARENT:
+		return true;
+	default:
+		BUG();
 	}
-
-	return true;
 }
 
 /*
@@ -2658,11 +2668,11 @@  static bool rbd_obj_handle_write(struct rbd_obj_request *obj_req, int *result)
 	case RBD_OBJ_WRITE_COPYUP_OPS:
 		return true;
 	case RBD_OBJ_WRITE_READ_FROM_PARENT:
-		if (*result < 0)
+		if (*result)
 			return true;
 
-		rbd_assert(*result);
-		ret = rbd_obj_issue_copyup(obj_req, *result);
+		ret = rbd_obj_issue_copyup(obj_req,
+					   rbd_obj_img_extents_bytes(obj_req));
 		if (ret) {
 			*result = ret;
 			return true;
@@ -2757,7 +2767,7 @@  static void rbd_obj_handle_request(struct rbd_obj_request *obj_req, int result)
 	rbd_assert(img_req->result <= 0);
 	if (test_bit(IMG_REQ_CHILD, &img_req->flags)) {
 		obj_req = img_req->obj_request;
-		result = img_req->result ?: rbd_obj_img_extents_bytes(obj_req);
+		result = img_req->result;
 		rbd_img_request_put(img_req);
 		goto again;
 	}