ocfs2: make ocfs2_adjust_resv_from_alloc simple.

PATCH New Tao Ma 3 participants 7 messages Apr 6, 2010 – Apr 26, 2010 OCFS2

Share & tools

Tao Ma

author

2010-04-06 07:40 UTC #0 lore raw

PATCH 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?

So the only chance we have to adjust the reservation is that we haven't reached the end. With this patch, the function is more readable.

Cc Mark Fasheh <[email protected]>

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/reservations.c |   33 ++++++++++-----------------------
 1 files changed, 10 insertions(+), 23 deletions(-)

fs/ocfs2/reservations.c +10 ~~-23~~

index 32bad4a..d841535 100644
@@ -406,7 +406,7 @@ ocfs2_find_resv_lhs(struct ocfs2_reservation_map *resmap, unsigned int goal)
  * The start value of *rstart is insignificant.
  *
  * This function searches the bitmap range starting at search_start
- * with length csearch_len for a set of contiguous free bits. We try
+ * with length search_len for a set of contiguous free bits. We try
  * to find up to 'wanted' bits, but can sometimes return less.
  *
  * Returns the length of allocation, 0 if no free bits are found.

@@ -776,38 +776,25 @@ static void
 				     struct ocfs2_alloc_reservation *resv,
 				     unsigned int start, unsigned int end)
 {
-	unsigned int lhs = 0, rhs = 0;
+	unsigned int rhs = 0;
+	unsigned int old_end = ocfs2_resv_end(resv);
 
-	BUG_ON(start < resv->r_start);
+	BUG_ON(start != resv->r_start || old_end < end);
 
-	/*
-	 * Completely used? We can remove it then.
-	 */
-	if (ocfs2_resv_end(resv) <= end && resv->r_start >= start) {
+	if (old_end == end) {
 		__ocfs2_resv_discard(resmap, resv);
 		return;
 	}
 
-	if (end < ocfs2_resv_end(resv))
-		rhs = end - ocfs2_resv_end(resv);
-
-	if (start > resv->r_start)
-		lhs = start - resv->r_start;
+	rhs = end - ocfs2_resv_end(resv);
 
 	/*
-	 * This should have been trapped above. At the very least, rhs
-	 * should be non zero.
+	 * This should have been trapped above.
 	 */
-	BUG_ON(rhs == 0 && lhs == 0);
+	BUG_ON(rhs == 0);
 
-	if (rhs >= lhs) {
-		unsigned int old_end = ocfs2_resv_end(resv);
-
-		resv->r_start = end + 1;
-		resv->r_len = old_end - resv->r_start + 1;
-	} else {
-		resv->r_len = start - resv->r_start;
-	}
+	resv->r_start = end + 1;
+	resv->r_len = old_end - resv->r_start + 1;
 }
 
 void ocfs2_resmap_claimed_bits(struct ocfs2_reservation_map *resmap,

Signature · 1 line

1.5.5

Mark Fasheh

2010-04-07 21:24 UTC #1 lore raw

PATCH 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

Quoted 6 lines · Tao Ma

When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?

I wanted ocfs2_resmap_claimed_bits() to be able to accept as many types of input as possible. That way, it would stay ready to handle other types of bitmaps. You're correct however that it's not really practical to allow for allocation which would split our reservation.

Btw, if we took this to it's logical conclusion I think we'd be able to get rid of the 'cstart' variable to ocfs2_resmap_claimed_bits(). It's good to at least track what the caller *thought* the start of the allocation was so IMHO we should leave that part of the api untouched.

Can you please add a comment in reservations.h that ocfs2_resmap_claimed_bits() expects that 'start' is the same as we passed back from ocfs2_resmap_resv_bits(). Also, there's a check at the top of ocfs2_resmap_claimed_bits() which can become:

BUG_ON(cstart != resv->r_start);
	--Mark

--
Mark Fasheh

Tao Ma

author

2010-04-07 23:47 UTC #2 lore raw

PATCH 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

Quoted 15 lines · Mark Fasheh

Quoted 7 lines · Tao Ma
When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?
I wanted ocfs2_resmap_claimed_bits() to be able to accept as many types of input as possible. That way, it would stay ready to handle other types of bitmaps. You're correct however that it's not really practical to allow for allocation which would split our reservation.

Quoted 5 lines

Btw, if we took this to it's logical conclusion I think we'd be able to get rid of the 'cstart' variable to ocfs2_resmap_claimed_bits(). It's good to at least track what the caller *thought* the start of the allocation was so IMHO we should leave that part of the api untouched.

yeah, I just want to change your codes as less as possible. ;)

Quoted 8 lines

Can you please add a comment in reservations.h that ocfs2_resmap_claimed_bits() expects that 'start' is the same as we passed back from ocfs2_resmap_resv_bits(). Also, there's a check at the top of ocfs2_resmap_claimed_bits() which can become:
BUG_ON(cstart != resv->r_start);

OK, I will add them and resend the patch.

Regards,
Tao

Tao Ma

author

2010-04-08 08:33 UTC #3 lore raw

PATCH v2 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

In reply to Mark Fasheh

So the only chance we have to adjust the reservation is that we haven't reached the end. With this patch, the function is more readable.

Note:
btw, this patch also fixes an original bug in the function which I haven't found before.

	if (end < ocfs2_resv_end(resv))
		rhs = end - ocfs2_resv_end(resv);

This code is of course buggy. ;)

Cc Mark Fasheh <[email protected]>

Signed-off-by Tao Ma <[email protected]>

---
 fs/ocfs2/reservations.c |   32 ++++++++++++--------------------
 fs/ocfs2/reservations.h |    3 ++-
 2 files changed, 14 insertions(+), 21 deletions(-)

fs/ocfs2/reservations.c +12 ~~-20~~

index 32bad4a..4065002 100644
@@ -406,7 +406,7 @@ ocfs2_find_resv_lhs(struct ocfs2_reservation_map *resmap, unsigned int goal)
  * The start value of *rstart is insignificant.
  *
  * This function searches the bitmap range starting at search_start
- * with length csearch_len for a set of contiguous free bits. We try
+ * with length search_len for a set of contiguous free bits. We try
  * to find up to 'wanted' bits, but can sometimes return less.
  *
  * Returns the length of allocation, 0 if no free bits are found.

@@ -776,38 +776,28 @@ static void
 				     struct ocfs2_alloc_reservation *resv,
 				     unsigned int start, unsigned int end)
 {
-	unsigned int lhs = 0, rhs = 0;
+	unsigned int rhs = 0;
+	unsigned int old_end = ocfs2_resv_end(resv);
 
-	BUG_ON(start < resv->r_start);
+	BUG_ON(start != resv->r_start || old_end < end);
 
 	/*
 	 * Completely used? We can remove it then.
 	 */
-	if (ocfs2_resv_end(resv) <= end && resv->r_start >= start) {
+	if (old_end == end) {
 		__ocfs2_resv_discard(resmap, resv);
 		return;
 	}
 
-	if (end < ocfs2_resv_end(resv))
-		rhs = end - ocfs2_resv_end(resv);
-
-	if (start > resv->r_start)
-		lhs = start - resv->r_start;
+	rhs = old_end - end;
 
 	/*
-	 * This should have been trapped above. At the very least, rhs
-	 * should be non zero.
+	 * This should have been trapped above.
 	 */
-	BUG_ON(rhs == 0 && lhs == 0);
-
-	if (rhs >= lhs) {
-		unsigned int old_end = ocfs2_resv_end(resv);
+	BUG_ON(rhs == 0);
 
-		resv->r_start = end + 1;
-		resv->r_len = old_end - resv->r_start + 1;
-	} else {
-		resv->r_len = start - resv->r_start;
-	}
+	resv->r_start = end + 1;
+	resv->r_len = old_end - resv->r_start + 1;
 }
 
 void ocfs2_resmap_claimed_bits(struct ocfs2_reservation_map *resmap,

@@ -822,6 +812,8 @@ void ocfs2_resmap_claimed_bits(struct ocfs2_reservation_map *resmap,
 	if (resv == NULL)
 		return;
 
+	BUG_ON(cstart != resv->r_start);
+
 	spin_lock(&resv_lock);
 
 	mlog(0, "claim bits: cstart: %u cend: %u clen: %u r_start: %u "

fs/ocfs2/reservations.h +2 -1

index 25b0c0e..1e49cc2 100644
@@ -149,7 +149,8 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
  * reservation. But we must always call this function when bits are claimed.
  * Internally, the reservations code will use this information to mark the
  * reservations bitmap. If resv is passed, it's next allocation window will be
- * calculated.
+ * calculated. It also expects that 'cstart' is the same as we passed back
+ * from ocfs2_resmap_resv_bits().
  */
 void ocfs2_resmap_claimed_bits(struct ocfs2_reservation_map *resmap,
 			       struct ocfs2_alloc_reservation *resv,

Signature · 1 line

1.5.5

Mark Fasheh

2010-04-23 21:44 UTC #4 lore raw

PATCH v2 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

Quoted 30 lines · Tao Ma

When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?
So the only chance we have to adjust the reservation is that we haven't reached the end. With this patch, the function is more readable.
Note:
btw, this patch also fixes an original bug in the function which I haven't found before.
	if (end < ocfs2_resv_end(resv))
		rhs = end - ocfs2_resv_end(resv);
This code is of course buggy. ;)
Cc Mark Fasheh <[email protected]>
Signed-off-by Tao Ma <[email protected]>
---
 fs/ocfs2/reservations.c |   32 ++++++++++++--------------------
 fs/ocfs2/reservations.h |    3 ++-
 2 files changed, 14 insertions(+), 21 deletions(-)
fs/ocfs2/reservations.c +0 -0
index 32bad4a..4065002 100644

fs/ocfs2/reservations.c ocfs2_find_resv_lhs(struct ocfs2_reservation_map *resmap, unsigned int goal) · line 412

Quoted 8 lines

fs/ocfs2/reservations.c +1 -1

@@ -406,7 +406,7 @@ ocfs2_find_resv_lhs(struct ocfs2_reservation_map *resmap, unsigned int goal)
  * The start value of *rstart is insignificant.
  *
  * This function searches the bitmap range starting at search_start
- * with length csearch_len for a set of contiguous free bits. We try
+ * with length search_len for a set of contiguous free bits. We try
  * to find up to 'wanted' bits, but can sometimes return less.
  *
  * Returns the length of allocation, 0 if no free bits are found.

This isn't related to the ocfs2_adjust_resv_from_alloc(), right? ;) Good catch on the comment cleanup though.

Otherwise the patch looks great. Joel, I personally don't care about the extra cleanup
- this *is* a cleanup patch anyway.

Acked-by Mark Fasheh <[email protected]>

--
Mark Fasheh

Joel Becker

2010-04-23 22:03 UTC #5 lore raw

PATCH v2 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

In reply to Tao Ma

Quoted 14 lines · Tao Ma

When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?
So the only chance we have to adjust the reservation is that we haven't reached the end. With this patch, the function is more readable.
Note:
btw, this patch also fixes an original bug in the function which I haven't found before.

Quoted 6 lines

	if (end < ocfs2_resv_end(resv))
		rhs = end - ocfs2_resv_end(resv);
This code is of course buggy. ;)
Cc Mark Fasheh <[email protected]>
Signed-off-by Tao Ma <[email protected]>

This patch is now in the 'merge-window' branch of ocfs2.git.

Joel

Signature · 9 lines

Life's Little Instruction Book #24

	"Drink champagne for no reason at all."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Tao Ma

author

2010-04-26 06:45 UTC #6 lore raw

PATCH v2 1/1 ocfs2: make ocfs2_adjust_resv_from_alloc simple.

Quoted 19 lines · Joel Becker

Quoted 18 lines · Tao Ma
When we allocate some bits from the reservation, we always allocate from the r_start(see ocfs2_resmap_resv_bits). So there should be no sense for checking between r_start and start. And I don't think we will change this behaviour somehow later by allocating from some bits after r_start. Why not make ocfs2_adjust_resv_from_alloc simple now?
So the only chance we have to adjust the reservation is that we haven't reached the end. With this patch, the function is more readable.
Note:
btw, this patch also fixes an original bug in the function which I haven't found before.
	if (end < ocfs2_resv_end(resv))
		rhs = end - ocfs2_resv_end(resv);
This code is of course buggy. ;)

Quoted 4 lines

Quoted 2 lines
Cc Mark Fasheh <[email protected]>
Signed-off-by Tao Ma <[email protected]>
This patch is now in the 'merge-window' branch of ocfs2.git.

thanks, joel.

btw, you still miss another patch which Mark has acked for the merge-window.
http://oss.oracle.com/pipermail/ocfs2-devel/2010-April/006250.html

Regards,
Tao