@@ -77,7 +77,7 @@ out_drop:
/*
* Verify that dentry is valid.
*
- * Should return 1 if dentry can still be trusted, else 0
+ * Should return 1 if dentry can still be trusted, else 0.
*/
static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
{
@@ -92,49 +92,27 @@ static int orangefs_d_revalidate(struct dentry *dentry, unsi
/* find inode from dentry */
if (!dentry->d_inode) {
- gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: negative dentry.\n",
+ gossip_debug(GOSSIP_DCACHE_DEBUG,
+ "%s: negative dentry.\n",
__func__);
- goto invalid_exit;
+ goto out;
}
gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: inode valid.\n", __func__);
inode = dentry->d_inode;
- /*
- * first perform a lookup to make sure that the object not only
- * exists, but is still in the expected place in the name space
- */
- if (!is_root_handle(inode)) {
- if (!orangefs_revalidate_lookup(dentry))
- goto invalid_exit;
- } else {
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: root handle, lookup skipped.\n",
- __func__);
+ /* skip root handle lookups. */
+ if (is_root_handle(inode)) {
+ ret = 1;
+ goto out;
}
- /* now perform getattr */
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: doing getattr: inode: %p, handle: %pU\n",
- __func__,
- inode,
- get_khandle_from_ino(inode));
- ret = orangefs_inode_getattr(inode, ORANGEFS_ATTR_SYS_ALL_NOHINT);
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: getattr %s (ret = %d), returning %s for dentry i_count
- __func__,
- (ret == 0 ? "succeeded" : "failed"),
- ret,
- (ret == 0 ? "valid" : "INVALID"),
- atomic_read(&inode->i_count));
- if (ret != 0)
- goto invalid_exit;
-
- /* dentry is valid! */
- return 1;
-
-invalid_exit:
- return 0;
+ /* lookup the object. */
+ if (orangefs_revalidate_lookup(dentry))
+ ret = 1;
+
+out:
+ return ret;
}
const struct dentry_operations orangefs_dentry_operations = {
All... WRT: fs/orangefs/symlink.c... AV> Said that, there is an unpleasant bug in that area - link_target of a live AV> inode can be overwritten, right under the pathname resolution walking the AV> old contents of that thing. AV> copy_attributes_to_inode() is triggered by ->d_revalidate() and AV> by ->getattr() and it's really, really unsafe for a live inode. I can see how that would be unsafe, but have had to look at a lot of other people's code, and in the Documentation directory, and think a lot (ouch it hurts when I do that) to see how to try and make it safe. ./filesystems/vfs.txt d_revalidate: called when the VFS needs to revalidate a dentry. This is called whenever a name look-up finds a dentry in the dcache. Most local filesystems leave this as NULL, because all their dentries in the dcache are valid. Network filesystems are different since things can change on the server without the client necessarily being aware of it. This function should return a positive value if the dentry is still valid, and zero or a negative error code if it isn't. Perhaps d_revalidate didn't have any business triggering copy_attributes_to_inode? I've made this change, and have no regressions with dbench or other silly tests, I'm about to leave the office for the afternoon with xfstests running. Do you think this change fixes the bug? $ git diff -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html