Skip to contentSkip to conversation

Fix inode transition from unlinked to free

PATCH New Bob Peterson 2 participants 7 messages Apr 27, 2016 – Apr 28, 2016 GFS2 file system

lore
Share & tools
lore
#0 lore raw

cover Fix inode transition from unlinked to free

This is a set of three patches designed to fix the severely broken transition from unlinked to free dinodes. Previously, I had posted patches to ditch the code that filtered out I_FREEing inodes, but now I'm adding it back in and leveraging it for the new patch. The new patch basically adds retry loops to the glock delete_work_func, so that when an inode is not found in memory, the operation is retried, but only if it's in an I_FREEing state. So the delete_work_func now
becomes: (1) Try to find the inode in inode cache (gfs2_ilookup). If the inode is in I_FREEING, we sleep and try again later, for a certain retry period. After that, if it's still not found in cache (whether not in cache or any other error) it goes on to call the gfs2_lookup_by_inum for a certain number of retries. Again, if the inode in still I_FREEing, we sleep and retry the operation. Since there's so much common logic, I decided to combine the two lookup functions (gfs2_ilookup and gfs2_iget) into a common function, inode_lookup_common. I also had to add the logic for the lookup to return -EAGAIN in the cases we need.

---
Bob Peterson (3):
Revert "GFS2: Eliminate parameter non_block on gfs2_inode_lookup" Revert "GFS2: Don't filter out I_FREEING inodes anymore"
GFS2: Add retry loop to delete_work_func

 fs/gfs2/dir.c        |  2 +-
 fs/gfs2/export.c     |  2 +-
 fs/gfs2/glock.c      | 29 ++++++++++++++------
 fs/gfs2/inode.c      | 77 ++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/gfs2/inode.h      |  5 ++--
 fs/gfs2/ops_fstype.c |  2 +-
 6 files changed, 99 insertions(+), 18 deletions(-)
Signature · 1 line
2.5.5
#1 lore raw

PATCH 1/3 Revert "GFS2: Eliminate parameter non_block on gfs2_inode_lookup"

This reverts commit 73b462d2808d7cbca4d7886cf6aaed850640e6cd.

---
 fs/gfs2/dir.c        | 2 +-
 fs/gfs2/inode.c      | 5 +++--
 fs/gfs2/inode.h      | 3 ++-
 fs/gfs2/ops_fstype.c | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)
fs/gfs2/dir.c +1 -1
index 4a01f30..d4014af 100644
@@ -1660,7 +1660,7 @@ struct inode *gfs2_dir_search(struct inode *dir, const struct qstr *name,
 		brelse(bh);
 		if (fail_on_exist)
 			return ERR_PTR(-EEXIST);
-		inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino);
+		inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino, 0);
 		if (!IS_ERR(inode))
 			GFS2_I(inode)->i_rahead = rahead;
 		return inode;
fs/gfs2/inode.c +3 -2
index aea002e..be6036a 100644
@@ -80,12 +80,13 @@ static void gfs2_set_iop(struct inode *inode)
  * @sb: The super block
  * @no_addr: The inode number
  * @type: The type of the inode
+ * non_block: Can we block on inodes that are being freed?
  *
  * Returns: A VFS inode, or an error
  */
 
 struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
-				u64 no_addr, u64 no_formal_ino)
+				u64 no_addr, u64 no_formal_ino, int non_block)
 {
 	struct inode *inode;
 	struct gfs2_inode *ip;
@@ -169,7 +170,7 @@ struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
 	if (error)
 		goto fail;
 
-	inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0);
+	inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0, 1);
 	if (IS_ERR(inode))
 		goto fail;
 
fs/gfs2/inode.h +2 -1
index e1af0d4..22c27a8 100644
@@ -94,7 +94,8 @@ err:
 }
 
 extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type, 
-				       u64 no_addr, u64 no_formal_ino);
+				       u64 no_addr, u64 no_formal_ino,
+				       int non_block);
 extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
 					 u64 *no_formal_ino,
 					 unsigned int blktype);
fs/gfs2/ops_fstype.c +1 -1
index 49b0bff..dbed9e2 100644
@@ -454,7 +454,7 @@ static int gfs2_lookup_root(struct super_block *sb, struct dentry **dptr,
 	struct dentry *dentry;
 	struct inode *inode;
 
-	inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0);
+	inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0, 0);
 	if (IS_ERR(inode)) {
 		fs_err(sdp, "can't read in %s inode: %ld\n", name, PTR_ERR(inode));
 		return PTR_ERR(inode);
Signature · 1 line
2.5.5
#2 lore raw

PATCH 2/3 Revert "GFS2: Don't filter out I_FREEING inodes anymore"

In reply to Bob Peterson

This reverts commit ff34245d524a898eee6e013eb1ec165095277148.

---
 fs/gfs2/export.c |  2 +-
 fs/gfs2/glock.c  |  2 +-
 fs/gfs2/inode.c  | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 fs/gfs2/inode.h  |  2 +-
 4 files changed, 58 insertions(+), 6 deletions(-)
fs/gfs2/export.c +1 -1
index d5bda85..5d15e94 100644
@@ -137,7 +137,7 @@ static struct dentry *gfs2_get_dentry(struct super_block *sb,
 	struct gfs2_sbd *sdp = sb->s_fs_info;
 	struct inode *inode;
 
-	inode = gfs2_ilookup(sb, inum->no_addr);
+	inode = gfs2_ilookup(sb, inum->no_addr, 0);
 	if (inode) {
 		if (GFS2_I(inode)->i_no_formal_ino != inum->no_formal_ino) {
 			iput(inode);
fs/gfs2/glock.c +1 -1
index 3910cea..672de35 100644
@@ -589,7 +589,7 @@ static void delete_work_func(struct work_struct *work)
 	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
 
 	if (ip)
-		inode = gfs2_ilookup(sdp->sd_vfs, no_addr);
+		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
 	else
 		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
 	if (inode && !IS_ERR(inode)) {
fs/gfs2/inode.c +55 -3

71 lines not on this page · raw message

fs/gfs2/inode.h +1 -1
index 22c27a8..ba4d949 100644
@@ -99,7 +99,7 @@ extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type,
 extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
 					 u64 *no_formal_ino,
 					 unsigned int blktype);
-extern struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr);
+extern struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int nonblock);
 
 extern int gfs2_inode_refresh(struct gfs2_inode *ip);
 
Signature · 1 line
2.5.5
#3 lore raw

PATCH 3/3 GFS2: Add retry loop to delete_work_func

In reply to Bob Peterson

The delete work function, delete_work_func, often doesn't find the inode needed to free an inode that's been marked unlinked. That's because it only tries gfs2_ilookup once, and it's often not found because of two things: The fact that gfs2_lookup_by_inum is only called in an else condition, and the fact that the non-blocking lookup often encounters inodes that are being I_FREEd by the vfs. This patch allows it to retry the lookup when under that circumstance, otherwise call the gfs2_lookup_by_inum.

---
 fs/gfs2/glock.c | 29 +++++++++++++++++++++--------
 fs/gfs2/inode.c | 40 +++++++++++++++++++++++++++-------------
 2 files changed, 48 insertions(+), 21 deletions(-)
fs/gfs2/glock.c +21 -8
index 672de35..06b77cc 100644
@@ -571,13 +571,15 @@ out_unlock:
 	return;
 }
 
+#define LOOKUP_RETRIES 30
+
 static void delete_work_func(struct work_struct *work)
 {
 	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct gfs2_inode *ip;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	u64 no_addr = gl->gl_name.ln_number;
+	int retries;
 
 	/* If someone's using this glock to create a new dinode, the block must
 	   have been freed by another node, then re-used, in which case our
@@ -585,13 +587,24 @@ static void delete_work_func(struct work_struct *work)
 	if (test_bit(GLF_INODE_CREATING, &gl->gl_flags))
 		goto out;
 
-	ip = gl->gl_object;
-	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
-
-	if (ip)
+	for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
 		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
-	else
-		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
+
+		if (inode == ERR_PTR(-EAGAIN))
+			msleep(10);
+		else
+			break;
+	}
+	if (inode == NULL) {
+		for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
+			inode = gfs2_lookup_by_inum(sdp, no_addr, NULL,
+						    GFS2_BLKST_UNLINKED);
+			if (inode == ERR_PTR(-EAGAIN))
+				msleep(10);
+			else
+				break;
+		}
+	}
 	if (inode && !IS_ERR(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
fs/gfs2/inode.c +27 -13
index 48c1418..3cd06c7 100644
@@ -37,6 +37,11 @@
 #include "super.h"
 #include "glops.h"
 
+enum {
+	LOOKUP_UNLOCKED = 0,
+	LOOKUP_LOCKED = 1,
+};
+
 struct gfs2_skip_data {
 	u64 no_addr;
 	int skipped;
@@ -71,27 +76,34 @@ static int iget_set(struct inode *inode, void *opaque)
 	return 0;
 }
 
-struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+static struct inode *inode_lookup_common(struct super_block *sb, u64 no_addr,
+					 int non_block, int locked)
 {
 	unsigned long hash = (unsigned long)no_addr;
-	struct gfs2_skip_data data;
+	struct gfs2_skip_data data = {.no_addr = no_addr, .skipped = 0,
+				      .non_block = non_block};
+	struct inode *inode;
 
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return ilookup5(sb, hash, iget_test, &data);
+	if (locked == LOOKUP_LOCKED)
+		inode = iget5_locked(sb, hash, iget_test, iget_set, &data);
+	else
+		inode = ilookup5(sb, hash, iget_test, &data);
+
+	if (non_block && data.skipped)
+		return ERR_PTR(-EAGAIN);
+
+	return inode;
+}
+
+struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+{
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_UNLOCKED);
 }
 
 static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr,
 			       int non_block)
 {
-	struct gfs2_skip_data data;
-	unsigned long hash = (unsigned long)no_addr;
-
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return iget5_locked(sb, hash, iget_test, iget_set, &data);
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_LOCKED);
 }
 
 /**
@@ -148,6 +160,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 	inode = gfs2_iget(sb, no_addr, non_block);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
+	if (inode == ERR_PTR(-EAGAIN))
+		return inode;
 
 	ip = GFS2_I(inode);
 	ip->i_no_addr = no_addr;
Signature · 1 line
2.5.5
#4 lore raw

PATCH v2 3/3 GFS2: Add retry loop to delete_work_func

Hi,

I made a slight correction to the third patch I posted previously. I added the condition " || IS_ERR(inode)" to the delete_work_func, which needs to be there. This is the corrected version.

The delete work function, delete_work_func, often doesn't find the inode needed to free an inode that's been marked unlinked. That's because it only tries gfs2_ilookup once, and it's often not found because of two things: The fact that gfs2_lookup_by_inum is only called in an else condition, and the fact that the non-blocking lookup often encounters inodes that are being I_FREEd by the vfs. This patch allows it to retry the lookup when under that circumstance, otherwise call the gfs2_lookup_by_inum.

---

fs/gfs2/glock.c +21 -8
index 672de35..0266b7c 100644
@@ -571,13 +571,15 @@ out_unlock:
 	return;
 }
 
+#define LOOKUP_RETRIES 30
+
 static void delete_work_func(struct work_struct *work)
 {
 	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct gfs2_inode *ip;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	u64 no_addr = gl->gl_name.ln_number;
+	int retries;
 
 	/* If someone's using this glock to create a new dinode, the block must
 	   have been freed by another node, then re-used, in which case our
@@ -585,13 +587,24 @@ static void delete_work_func(struct work_struct *work)
 	if (test_bit(GLF_INODE_CREATING, &gl->gl_flags))
 		goto out;
 
-	ip = gl->gl_object;
-	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
-
-	if (ip)
+	for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
 		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
-	else
-		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
+
+		if (inode == ERR_PTR(-EAGAIN))
+			msleep(10);
+		else
+			break;
+	}
+	if (inode == NULL || IS_ERR(inode)) {
+		for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
+			inode = gfs2_lookup_by_inum(sdp, no_addr, NULL,
+						    GFS2_BLKST_UNLINKED);
+			if (inode == ERR_PTR(-EAGAIN))
+				msleep(10);
+			else
+				break;
+		}
+	}
 	if (inode && !IS_ERR(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
fs/gfs2/inode.c +27 -13
index 48c1418..3cd06c7 100644
@@ -37,6 +37,11 @@
 #include "super.h"
 #include "glops.h"
 
+enum {
+	LOOKUP_UNLOCKED = 0,
+	LOOKUP_LOCKED = 1,
+};
+
 struct gfs2_skip_data {
 	u64 no_addr;
 	int skipped;
@@ -71,27 +76,34 @@ static int iget_set(struct inode *inode, void *opaque)
 	return 0;
 }
 
-struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+static struct inode *inode_lookup_common(struct super_block *sb, u64 no_addr,
+					 int non_block, int locked)
 {
 	unsigned long hash = (unsigned long)no_addr;
-	struct gfs2_skip_data data;
+	struct gfs2_skip_data data = {.no_addr = no_addr, .skipped = 0,
+				      .non_block = non_block};
+	struct inode *inode;
 
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return ilookup5(sb, hash, iget_test, &data);
+	if (locked == LOOKUP_LOCKED)
+		inode = iget5_locked(sb, hash, iget_test, iget_set, &data);
+	else
+		inode = ilookup5(sb, hash, iget_test, &data);
+
+	if (non_block && data.skipped)
+		return ERR_PTR(-EAGAIN);
+
+	return inode;
+}
+
+struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+{
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_UNLOCKED);
 }
 
 static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr,
 			       int non_block)
 {
-	struct gfs2_skip_data data;
-	unsigned long hash = (unsigned long)no_addr;
-
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return iget5_locked(sb, hash, iget_test, iget_set, &data);
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_LOCKED);
 }
 
 /**
@@ -148,6 +160,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 	inode = gfs2_iget(sb, no_addr, non_block);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
+	if (inode == ERR_PTR(-EAGAIN))
+		return inode;
 
 	ip = GFS2_I(inode);
 	ip->i_no_addr = no_addr;
#5 lore raw

PATCH v2 3/3 GFS2: Add retry loop to delete_work_func

Hi,

Quoted 47 lines · Bob Peterson

Hi,

I made a slight correction to the third patch I posted previously. I added the condition " || IS_ERR(inode)" to the delete_work_func, which needs to be there. This is the corrected version.

The delete work function, delete_work_func, often doesn't find the inode needed to free an inode that's been marked unlinked. That's because it only tries gfs2_ilookup once, and it's often not found because of two things: The fact that gfs2_lookup_by_inum is only called in an else condition, and the fact that the non-blocking lookup often encounters inodes that are being I_FREEd by the vfs. This patch allows it to retry the lookup when under that circumstance, otherwise call the gfs2_lookup_by_inum.

---

fs/gfs2/glock.c +4 -6
index 672de35..0266b7c 100644
@@ -571,13 +571,15 @@ out_unlock:
  	return;
  }
 
+#define LOOKUP_RETRIES 30
+
  static void delete_work_func(struct work_struct *work)
  {
  	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
  	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct gfs2_inode *ip;
-	struct inode *inode;
+	struct inode *inode = NULL;
  	u64 no_addr = gl->gl_name.ln_number;
+	int retries;
 
  	/* If someone's using this glock to create a new dinode, the block must
  	   have been freed by another node, then re-used, in which case our
@@ -585,13 +587,24 @@ static void delete_work_func(struct work_struct *work)
  	if (test_bit(GLF_INODE_CREATING, &gl->gl_flags))
  		goto out;
 
-	ip = gl->gl_object;
-	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
-
-	if (ip)

fs/gfs2/glock.c static void delete_work_func(struct work_struct *work) · line 597

Quoted 10 lines
fs/gfs2/glock.c +7 -2
+	for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
  		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
-	else
-		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
+
+		if (inode == ERR_PTR(-EAGAIN))
+			msleep(10);
+		else
+			break;
+	}

This generally looks like a much better solution, however I'm always worried about arbitrary delays being added to the code. Is this just a wait for an inode in I_FREEING to go away, along with a timeout? It would be good to at least document why 30 loops here is the right amount. In other words will this still be sure to work on different machines with different timing characteristics?

I wonder whether it might not be better to reschedule the work function for later, rather than loop in the work function itself?

Steve.

Quoted 87 lines
fs/gfs2/glock.c +10 -0
+	if (inode == NULL || IS_ERR(inode)) {
+		for (retries = 0; retries < LOOKUP_RETRIES; retries++) {
+			inode = gfs2_lookup_by_inum(sdp, no_addr, NULL,
+						    GFS2_BLKST_UNLINKED);
+			if (inode == ERR_PTR(-EAGAIN))
+				msleep(10);
+			else
+				break;
+		}
+	}
  	if (inode && !IS_ERR(inode)) {
  		d_prune_aliases(inode);
  		iput(inode);
fs/gfs2/inode.c +27 -13
index 48c1418..3cd06c7 100644
@@ -37,6 +37,11 @@
  #include "super.h"
  #include "glops.h"
 
+enum {
+	LOOKUP_UNLOCKED = 0,
+	LOOKUP_LOCKED = 1,
+};
+
  struct gfs2_skip_data {
  	u64 no_addr;
  	int skipped;
@@ -71,27 +76,34 @@ static int iget_set(struct inode *inode, void *opaque)
  	return 0;
  }
 
-struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+static struct inode *inode_lookup_common(struct super_block *sb, u64 no_addr,
+					 int non_block, int locked)
  {
  	unsigned long hash = (unsigned long)no_addr;
-	struct gfs2_skip_data data;
+	struct gfs2_skip_data data = {.no_addr = no_addr, .skipped = 0,
+				      .non_block = non_block};
+	struct inode *inode;
 
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return ilookup5(sb, hash, iget_test, &data);
+	if (locked == LOOKUP_LOCKED)
+		inode = iget5_locked(sb, hash, iget_test, iget_set, &data);
+	else
+		inode = ilookup5(sb, hash, iget_test, &data);
+
+	if (non_block && data.skipped)
+		return ERR_PTR(-EAGAIN);
+
+	return inode;
+}
+
+struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+{
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_UNLOCKED);
  }
 
  static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr,
  			       int non_block)
  {
-	struct gfs2_skip_data data;
-	unsigned long hash = (unsigned long)no_addr;
-
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return iget5_locked(sb, hash, iget_test, iget_set, &data);
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_LOCKED);
  }
 
  /**
@@ -148,6 +160,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
  	inode = gfs2_iget(sb, no_addr, non_block);
  	if (!inode)
  		return ERR_PTR(-ENOMEM);
+	if (inode == ERR_PTR(-EAGAIN))
+		return inode;
 
  	ip = GFS2_I(inode);
  	ip->i_no_addr = no_addr;
#6 lore raw

PATCH v3 3/3 GFS2: Add retry loop to delete_work_func

Hi Steve,

Comments below.

----- Original Message -----

Quoted 11 lines

This generally looks like a much better solution, however I'm always worried about arbitrary delays being added to the code. Is this just a wait for an inode in I_FREEING to go away, along with a timeout? It would be good to at least document why 30 loops here is the right amount. In other words will this still be sure to work on different machines with different timing characteristics?

I wonder whether it might not be better to reschedule the work function for later, rather than loop in the work function itself?

Steve.

1. I chose 30 iterations arbitrarily based on instrumentation on the virt cluster I was using. I never saw it go over 29 retries. In my experience, virt clusters are faster and have more critical timing than bare metal, so I considered this worst case. But it is still arbitrary.
2. Your idea of re-queuing the delete work is an excellent one, and I've reworked the patch to do this. I'm testing the implementation now and it seems to be working well so far. This time my retry value is 30 seconds, with a 10ms delay between tries, but this is still arbitrary, so I'm open to suggestions.

Here is the replacement patch:

Patch description:

The delete work function, delete_work_func, often doesn't find the inode needed to free an inode that's been marked unlinked. That's because it only tries gfs2_ilookup once, and it's often not found because of two things: The fact that gfs2_lookup_by_inum is only called in an else condition, and the fact that the non-blocking lookup often encounters inodes that are being I_FREEd by the vfs. This patch allows it to retry the lookup when under that circumstance, otherwise call the gfs2_lookup_by_inum. If the inode is in I_FREEING, -EAGAIN is returned to the caller, who then re-queues the delete work for later. After a certain timeout value, the delete_work_func stops using ilookup and uses lookup_by_inum instead. If that fails for a certain number of retries, it gives up.

---

fs/gfs2/glock.c +42 -10
index 672de35..59efb8b 100644
@@ -571,12 +571,14 @@ out_unlock:
 	return;
 }
 
+#define LOOKUP_TIMEOUT (HZ >> 1)
+
 static void delete_work_func(struct work_struct *work)
 {
-	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
+	struct gfs2_glock *gl = container_of(work, struct gfs2_glock,
+					     gl_delete.work);
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct gfs2_inode *ip;
-	struct inode *inode;
+	struct inode *inode = NULL;
 	u64 no_addr = gl->gl_name.ln_number;
 
 	/* If someone's using this glock to create a new dinode, the block must
@@ -585,13 +587,43 @@ static void delete_work_func(struct work_struct *work)
 	if (test_bit(GLF_INODE_CREATING, &gl->gl_flags))
 		goto out;
 
-	ip = gl->gl_object;
-	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
-
-	if (ip)
+	if (test_bit(GLF_TRY_ILOOKUP, &gl->gl_flags)) {
 		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
-	else
-		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
+
+		if (inode == ERR_PTR(-EAGAIN)) {
+			if (time_before(jiffies, gl->gl_tchange +
+					LOOKUP_TIMEOUT)) {
+				gfs2_glock_hold(gl);
+				if (queue_delayed_work(gfs2_delete_workqueue,
+						       &gl->gl_delete, 10) == 0)
+					gfs2_glock_put(gl);
+				goto out;
+			} else {
+				clear_bit(GLF_TRY_ILOOKUP, &gl->gl_flags);
+				gl->gl_tchange = jiffies;
+			}
+		}
+	}
+
+	if (inode == NULL || IS_ERR(inode)) {
+		/* Note: This function uses the iopen glock only. It relies on
+		   the fact that gfs2_inode_lookup (called by lookup_by_inum)
+		   will return -EAGAIN before it does any manipulation of the
+		   iopen glock that might change gl_tchange. */
+		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL,
+					    GFS2_BLKST_UNLINKED);
+		if (inode == ERR_PTR(-EAGAIN)) {
+			if (time_before(jiffies, gl->gl_tchange +
+					LOOKUP_TIMEOUT)) {
+				gfs2_glock_hold(gl);
+				if (queue_delayed_work(gfs2_delete_workqueue,
+						       &gl->gl_delete, 10) == 0) {
+					gfs2_glock_put(gl);
+				}
+			}
+			goto out;
+		}
+	}
 	if (inode && !IS_ERR(inode)) {
 		d_prune_aliases(inode);
 		iput(inode);
@@ -713,7 +745,7 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	gl->gl_object = NULL;
 	gl->gl_hold_time = GL_GLOCK_DFT_HOLD;
 	INIT_DELAYED_WORK(&gl->gl_work, glock_work_func);
-	INIT_WORK(&gl->gl_delete, delete_work_func);
+	INIT_DELAYED_WORK(&gl->gl_delete, delete_work_func);
 
 	mapping = gfs2_glock2aspace(gl);
 	if (mapping) {
fs/gfs2/glops.c +4 -1
index 5db59d4..c0a45ed 100644
@@ -550,7 +550,10 @@ static void iopen_go_callback(struct gfs2_glock *gl, bool remote)
 	if (gl->gl_demote_state == LM_ST_UNLOCKED &&
 	    gl->gl_state == LM_ST_SHARED && ip) {
 		gl->gl_lockref.count++;
-		if (queue_work(gfs2_delete_workqueue, &gl->gl_delete) == 0)
+		gl->gl_tchange = jiffies;
+		set_bit(GLF_TRY_ILOOKUP, &gl->gl_flags);
+		if (queue_delayed_work(gfs2_delete_workqueue, &gl->gl_delete,
+			    0) == 0)
 			gl->gl_lockref.count--;
 	}
 }
fs/gfs2/incore.h +2 -1
index a6a3389..8c79fe4 100644
@@ -329,6 +329,7 @@ enum {
 	GLF_OBJECT			= 14, /* Used only for tracing */
 	GLF_BLOCKING			= 15,
 	GLF_INODE_CREATING		= 16, /* Inode creation occurring */
+	GLF_TRY_ILOOKUP			= 17, /* Try gfs2_ilookup for del */
 };
 
 struct gfs2_glock {
@@ -363,7 +364,7 @@ struct gfs2_glock {
 	struct delayed_work gl_work;
 	union {
 		/* For inode and iopen glocks only */
-		struct work_struct gl_delete;
+		struct delayed_work gl_delete;
 		/* For rgrp glocks only */
 		struct {
 			loff_t start;
fs/gfs2/inode.c +27 -13
index 48c1418..3cd06c7 100644
@@ -37,6 +37,11 @@
 #include "super.h"
 #include "glops.h"
 
+enum {
+	LOOKUP_UNLOCKED = 0,
+	LOOKUP_LOCKED = 1,
+};
+
 struct gfs2_skip_data {
 	u64 no_addr;
 	int skipped;
@@ -71,27 +76,34 @@ static int iget_set(struct inode *inode, void *opaque)
 	return 0;
 }
 
-struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+static struct inode *inode_lookup_common(struct super_block *sb, u64 no_addr,
+					 int non_block, int locked)
 {
 	unsigned long hash = (unsigned long)no_addr;
-	struct gfs2_skip_data data;
+	struct gfs2_skip_data data = {.no_addr = no_addr, .skipped = 0,
+				      .non_block = non_block};
+	struct inode *inode;
 
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return ilookup5(sb, hash, iget_test, &data);
+	if (locked == LOOKUP_LOCKED)
+		inode = iget5_locked(sb, hash, iget_test, iget_set, &data);
+	else
+		inode = ilookup5(sb, hash, iget_test, &data);
+
+	if (non_block && data.skipped)
+		return ERR_PTR(-EAGAIN);
+
+	return inode;
+}
+
+struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
+{
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_UNLOCKED);
 }
 
 static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr,
 			       int non_block)
 {
-	struct gfs2_skip_data data;
-	unsigned long hash = (unsigned long)no_addr;
-
-	data.no_addr = no_addr;
-	data.skipped = 0;
-	data.non_block = non_block;
-	return iget5_locked(sb, hash, iget_test, iget_set, &data);
+	return inode_lookup_common(sb, no_addr, non_block, LOOKUP_LOCKED);
 }
 
 /**
@@ -148,6 +160,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 	inode = gfs2_iget(sb, no_addr, non_block);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
+	if (inode == ERR_PTR(-EAGAIN))
+		return inode;
 
 	ip = GFS2_I(inode);
 	ip->i_no_addr = no_addr;
fs/gfs2/rgrp.c +4 -2
index 07c0265..58c74ca 100644
@@ -1801,8 +1801,10 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 		 * answer to whether it is NULL or not.
 		 */
 		ip = gl->gl_object;
-
-		if (ip || queue_work(gfs2_delete_workqueue, &gl->gl_delete) == 0)
+		gl->gl_tchange = jiffies;
+		set_bit(GLF_TRY_ILOOKUP, &gl->gl_flags);
+		if (ip || queue_delayed_work(gfs2_delete_workqueue,
+					     &gl->gl_delete, 0) == 0)
 			gfs2_glock_put(gl);
 		else
 			found++;