Skip to contentSkip to conversation

ocfs2: Cleanup dirent size check

PATCH New Mark Fasheh 2 participants 3 messages Sep 19, 2007 – Sep 20, 2007 OCFS2

lore
Share & tools
lore
#0 lore raw

PATCH 10/15 ocfs2: Cleanup dirent size check

The check to see if a new dirent would fit in an old one is pretty ugly, and it's done at least twice. Clean things up by putting this in it's own easier-to-read function.

---
 fs/ocfs2/dir.c |   36 ++++++++++++++++++++++++++++--------
 1 files changed, 28 insertions(+), 8 deletions(-)
fs/ocfs2/dir.c +28 -8
index 31db7e3..4c36eae 100644
@@ -343,6 +343,31 @@ bail:
 	return status;
 }
 
+/*
+ * Check whether 'de' has enough room to hold an entry of
+ * 'new_rec_len' bytes.
+ */
+static inline int ocfs2_dirent_would_fit(struct ocfs2_dir_entry *de,
+					 unsigned int new_rec_len)
+{
+	unsigned int de_really_needed;
+
+	/* Check whether this is an empty record with enough space */
+	if (le64_to_cpu(de->inode) == 0 &&
+	    le16_to_cpu(de->rec_len) >= new_rec_len)
+		return 1;
+
+	/*
+	 * Record might have free space at the end which we can
+	 * use.
+	 */
+	de_really_needed = OCFS2_DIR_REC_LEN(de->name_len);
+	if (le16_to_cpu(de->rec_len) >= (de_really_needed + new_rec_len))
+	    return 1;
+
+	return 0;
+}
+
 /* we don't always have a dentry for what we want to add, so people
  * like orphan dir can call this instead.
  *
@@ -385,10 +410,8 @@ int __ocfs2_add_entry(handle_t *handle,
 			retval = -EEXIST;
 			goto bail;
 		}
-		if (((le64_to_cpu(de->inode) == 0) &&
-		     (le16_to_cpu(de->rec_len) >= rec_len)) ||
-		    (le16_to_cpu(de->rec_len) >=
-		     (OCFS2_DIR_REC_LEN(de->name_len) + rec_len))) {
+
+		if (ocfs2_dirent_would_fit(de, rec_len)) {
 			dir->i_mtime = dir->i_ctime = CURRENT_TIME;
 			retval = ocfs2_mark_inode_dirty(handle, dir, parent_fe_bh);
 			if (retval < 0) {
@@ -1078,10 +1101,7 @@ int ocfs2_prepare_dir_for_insert(struct ocfs2_super *osb,
 			status = -EEXIST;
 			goto bail;
 		}
-		if (((le64_to_cpu(de->inode) == 0) &&
-		     (le16_to_cpu(de->rec_len) >= rec_len)) ||
-		    (le16_to_cpu(de->rec_len) >=
-		     (OCFS2_DIR_REC_LEN(de->name_len) + rec_len))) {
+		if (ocfs2_dirent_would_fit(de, rec_len)) {
 			/* Ok, we found a spot. Return this bh and let
 			 * the caller actually fill it in. */
 			*ret_de_bh = bh;
Signature · 1 line
1.5.0.6
#1 lore raw

PATCH 10/15 ocfs2: Cleanup dirent size check

Quoted 5 lines · Mark Fasheh

The check to see if a new dirent would fit in an old one is pretty ugly, and it's done at least twice. Clean things up by putting this in it's own easier-to-read function.

de_really_needed sounds like it's the new needed, which confused me (de_really_needed + new_rec_len sounds like "new needed + new needed", which of course makes no sense). Can you rename de_really_needed to de_really_used, de_old_used, or de_old_needed? That captures the sense of "how much space in the old dirent is actually used".

Quoted 73 lines
---
 fs/ocfs2/dir.c |   36 ++++++++++++++++++++++++++++--------
 1 files changed, 28 insertions(+), 8 deletions(-)
fs/ocfs2/dir.c +28 -8
index 31db7e3..4c36eae 100644
@@ -343,6 +343,31 @@ bail:
 	return status;
 }
 
+/*
+ * Check whether 'de' has enough room to hold an entry of
+ * 'new_rec_len' bytes.
+ */
+static inline int ocfs2_dirent_would_fit(struct ocfs2_dir_entry *de,
+					 unsigned int new_rec_len)
+{
+	unsigned int de_really_needed;
+
+	/* Check whether this is an empty record with enough space */
+	if (le64_to_cpu(de->inode) == 0 &&
+	    le16_to_cpu(de->rec_len) >= new_rec_len)
+		return 1;
+
+	/*
+	 * Record might have free space at the end which we can
+	 * use.
+	 */
+	de_really_needed = OCFS2_DIR_REC_LEN(de->name_len);
+	if (le16_to_cpu(de->rec_len) >= (de_really_needed + new_rec_len))
+	    return 1;
+
+	return 0;
+}
+
 /* we don't always have a dentry for what we want to add, so people
  * like orphan dir can call this instead.
  *
@@ -385,10 +410,8 @@ int __ocfs2_add_entry(handle_t *handle,
 			retval = -EEXIST;
 			goto bail;
 		}
-		if (((le64_to_cpu(de->inode) == 0) &&
-		     (le16_to_cpu(de->rec_len) >= rec_len)) ||
-		    (le16_to_cpu(de->rec_len) >=
-		     (OCFS2_DIR_REC_LEN(de->name_len) + rec_len))) {
+
+		if (ocfs2_dirent_would_fit(de, rec_len)) {
 			dir->i_mtime = dir->i_ctime = CURRENT_TIME;
 			retval = ocfs2_mark_inode_dirty(handle, dir, parent_fe_bh);
 			if (retval < 0) {
@@ -1078,10 +1101,7 @@ int ocfs2_prepare_dir_for_insert(struct ocfs2_super *osb,
 			status = -EEXIST;
 			goto bail;
 		}
-		if (((le64_to_cpu(de->inode) == 0) &&
-		     (le16_to_cpu(de->rec_len) >= rec_len)) ||
-		    (le16_to_cpu(de->rec_len) >=
-		     (OCFS2_DIR_REC_LEN(de->name_len) + rec_len))) {
+		if (ocfs2_dirent_would_fit(de, rec_len)) {
 			/* Ok, we found a spot. Return this bh and let
 			 * the caller actually fill it in. */
 			*ret_de_bh = bh;
Signature · 7 lines
1.5.0.6


_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
Signature · 9 lines
"You can get more with a kind word and a gun than you can with
 a kind word alone."
         - Al Capone

Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
#2 lore raw

PATCH 10/15 ocfs2: Cleanup dirent size check

Quoted 5 lines · Joel Becker
Quoted 4 lines · Mark Fasheh

The check to see if a new dirent would fit in an old one is pretty ugly, and it's done at least twice. Clean things up by putting this in it's own easier-to-read function.

Quoted 7 lines
Quoted 1 line

de_really_needed sounds like it's the new needed, which confused me (de_really_needed + new_rec_len sounds like "new needed + new needed", which of course makes no sense). Can you rename de_really_needed to de_really_used, de_old_used, or de_old_needed? That captures the sense of "how much space in the old dirent is actually used".

Yeah, that variable naming even confuses me when I look at it :(

I'm going to spin the wheel of variable names and land at.... de_really_used!

	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
[email protected]