ocfs2: code cleanup for .36

The first time I read the function ocfs2_resmap_resv_bits, I consider about what 'wanted' will be used and consider about the comments. Then I find it is only used if the reservation is empty. ;)

So we'd better move it to the parens so that it make the code more readable, what's more, ocfs2_resmap_resv_bits is used so frequently and we should save some cpus. The corresponding BUG_ON is also moved into parens since it is only meaningful after we reinit the resv.

Cc Mark Fasheh <[email protected]>

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

---
 fs/ocfs2/reservations.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

fs/ocfs2/reservations.c +10 -12

index d8b6e42..567c1a0 100644
@@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
 			   struct ocfs2_alloc_reservation *resv,
 			   int *cstart, int *clen)
 {
-	unsigned int wanted = *clen;
-
 	if (resv == NULL || ocfs2_resmap_disabled(resmap))
 		return -ENOSPC;
 
 	spin_lock(&resv_lock);
 
-	/*
-	 * We don't want to over-allocate for temporary
-	 * windows. Otherwise, we run the risk of fragmenting the
-	 * allocation space.
-	 */
-	wanted = ocfs2_resv_window_bits(resmap, resv);
-	if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
-		wanted = *clen;
-
 	if (ocfs2_resv_empty(resv)) {
-		mlog(0, "empty reservation, find new window\n");
+		/*
+		 * We don't want to over-allocate for temporary
+		 * windows. Otherwise, we run the risk of fragmenting the
+		 * allocation space.
+		 */
+		unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
+
+		if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
+			wanted = *clen;
 
+		mlog(0, "empty reservation, find new window\n");
 		/*
 		 * Try to get a window here. If it works, we must fall
 		 * through and test the bitmap . This avoids some

Hi Tao,

Quoted 61 lines · Tao Ma

The first time I read the function ocfs2_resmap_resv_bits, I consider about what 'wanted' will be used and consider about the comments. Then I find it is only used if the reservation is empty. ;)

So we'd better move it to the parens so that it make the code more readable, what's more, ocfs2_resmap_resv_bits is used so frequently and we should save some cpus. The corresponding BUG_ON is also moved into parens since it is only meaningful after we reinit the resv.

Cc Mark Fasheh<[email protected]>

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

---
  fs/ocfs2/reservations.c |   26 ++++++++++++--------------
  1 files changed, 12 insertions(+), 14 deletions(-)

fs/ocfs2/reservations.c +10 -13

index d8b6e42..567c1a0 100644
@@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
  			   struct ocfs2_alloc_reservation *resv,
  			   int *cstart, int *clen)
  {
-	unsigned int wanted = *clen;
-
  	if (resv == NULL || ocfs2_resmap_disabled(resmap))
  		return -ENOSPC;
 
  	spin_lock(&resv_lock);
 
-	/*
-	 * We don't want to over-allocate for temporary
-	 * windows. Otherwise, we run the risk of fragmenting the
-	 * allocation space.
-	 */
-	wanted = ocfs2_resv_window_bits(resmap, resv);
-	if ((resv->r_flags&  OCFS2_RESV_FLAG_TMP) || wanted<  *clen)
-		wanted = *clen;
-
  	if (ocfs2_resv_empty(resv)) {
-		mlog(0, "empty reservation, find new window\n");
+		/*
+		 * We don't want to over-allocate for temporary
+		 * windows. Otherwise, we run the risk of fragmenting the
+		 * allocation space.
+		 */
+		unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
+
+		if ((resv->r_flags&  OCFS2_RESV_FLAG_TMP) || wanted<  *clen)
+			wanted = *clen;
 
+		mlog(0, "empty reservation, find new window\n");
  		/*
  		 * Try to get a window here. If it works, we must fall
  		 * through and test the bitmap . This avoids some

@@ -759,9 +757,9 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
  		 * that inode.
  		 */
  		ocfs2_resv_find_window(resmap, resv, wanted);
-	}
 

fs/ocfs2/reservations.c int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap, · line 762

Quoted 3 lines

fs/ocfs2/reservations.c +2 -1

-	BUG_ON(ocfs2_resv_empty(resv));
+		BUG_ON(ocfs2_resv_empty(resv));
+	}

Can we leave the BUG_ON() outside the if (ocfs2_resv_empty(...)) { } block? You're technically correct in that the only possibility right now is if there's a bug in ocfs2_resv_find_window(). However, I think it still belongs outside the block because we're returning an allocation at that point. The code is self documenting then - "don't let it get here unless resv->r_start and resv->r_len mean something". That way any future changes to the function will be forced to take this BUG_ON() into consideration, and we are then less likely to accidentally corrupt data.

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

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

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

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