Re: [PATCH] autofs4: reinstate negatitive timeout of mount fails

[Ian Kent]

On Tue, 2007-08-21 at 10:19 -0400, Peter Staubach wrote:
> Ian Kent wrote:
> > Hi,
> >
> > Due to a change to fs/dcache.c:d_lookup() in the 2.6 kernel whereby only
> > hashed dentrys are returned the negative caching of mount failures
> > stopped working in the autofs4 module for nobrowse mount (ie. directory
> > created at mount time and removed at umount or following a mount
> > failure).
> >
> > This patch keeps track of the dentrys from mount fails in order to be
> > able check the timeout since the last fail and return the appropriate
> > status. In addition the timeout value is settable at load time as a
> > module option and via sysfs using the module
> > parameter /sys/module/autofs4/parameters/negative_timeout.
> >
> > Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
> >
> > ---
> > --- linux-2.6.23-rc2-mm2/fs/autofs4/init.c.negative-timeout	2007-07-09 07:32:17.000000000 +0800
> > +++ linux-2.6.23-rc2-mm2/fs/autofs4/init.c	2007-08-21 15:44:34.000000000 +0800
> > @@ -14,6 +14,10 @@
> >  #include <linux/init.h>
> >  #include "autofs_i.h"
> >  
> > +unsigned int negative_timeout = AUTOFS_NEGATIVE_TIMEOUT;
> > +module_param(negative_timeout, uint, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(negative_timeout, "Cache mount fails negatively for this many seconds");
> > +
> >  static int autofs_get_sb(struct file_system_type *fs_type,
> >  	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
> >  {
> > --- linux-2.6.23-rc2-mm2/fs/autofs4/inode.c.negative-timeout	2007-08-17 11:52:33.000000000 +0800
> > +++ linux-2.6.23-rc2-mm2/fs/autofs4/inode.c	2007-08-21 15:44:34.000000000 +0800
> > @@ -46,6 +46,7 @@ struct autofs_info *autofs4_init_ino(str
> >  	ino->inode = NULL;
> >  	ino->dentry = NULL;
> >  	ino->size = 0;
> > +	ino->negative_timeout = negative_timeout;
> >  
> >  	INIT_LIST_HEAD(&ino->rehash);
> >  
> > @@ -98,11 +99,24 @@ void autofs4_free_ino(struct autofs_info
> >  static void autofs4_force_release(struct autofs_sb_info *sbi)
> >  {
> >  	struct dentry *this_parent = sbi->sb->s_root;
> > -	struct list_head *next;
> > +	struct list_head *p, *next;
> >  
> >  	if (!sbi->sb->s_root)
> >  		return;
> >  
> > +	/* Cleanup the negative dentry cache */
> > +	spin_lock(&sbi->rehash_lock);
> > +	list_for_each_safe(p, next, &sbi->rehash_list) {
> > +		struct autofs_info *ino;
> > +		struct dentry *dentry;
> > +		ino = list_entry(p, struct autofs_info, rehash);
> > +		dentry = ino->dentry;
> > +		spin_unlock(&sbi->rehash_lock);
> > +		dput(ino->dentry);
> >   
> 
> Should this be dput(dentry);?

It could be since they're the same or maybe I should get rid of the
assignment. Maybe that would save a couple of cpu cycles.

> 
>     Thanx...
> 
>        ps
> 
> 
> > +		spin_lock(&sbi->rehash_lock);
> > +	}
> > +	spin_unlock(&sbi->rehash_lock);
> > +
> >  	spin_lock(&dcache_lock);
> >  repeat:
> >  	next = this_parent->d_subdirs.next;
> > --- linux-2.6.23-rc2-mm2/fs/autofs4/autofs_i.h.negative-timeout	2007-08-17 11:52:33.000000000 +0800
> > +++ linux-2.6.23-rc2-mm2/fs/autofs4/autofs_i.h	2007-08-21 15:44:34.000000000 +0800
> > @@ -40,6 +40,14 @@
> >  #define DPRINTK(fmt,args...) do {} while(0)
> >  #endif
> >  
> > +/*
> > + * If the daemon returns a negative response (AUTOFS_IOC_FAIL) then we keep
> > + * the negative response cached for up to the time given here, although
> > + * the time can be shorter if the kernel throws the dcache entry away.
> > + */
> > +#define AUTOFS_NEGATIVE_TIMEOUT	60	/* default 1 minute */
> > +extern unsigned int negative_timeout;
> > +
> >  /* Unified info structure.  This is pointed to by both the dentry and
> >     inode structures.  Each file in the filesystem has an instance of this
> >     structure.  It holds a reference to the dentry, so dentries are never
> > @@ -52,8 +60,16 @@ struct autofs_info {
> >  
> >  	int		flags;
> >  
> > +	/*
> > +	 * Two types of unhashed dentry can exist on this list.
> > +	 * Negative dentrys from failed mounts and positive dentrys
> > +	 * resulting from a race between expire and mount. This 
> > +	 * fact is used when looking for dentrys in the list.
> > +	 */
> >  	struct list_head rehash;
> >  
> > +	unsigned int negative_timeout;
> > +
> >  	struct autofs_sb_info *sbi;
> >  	unsigned long last_used;
> >  	atomic_t count;
> > --- linux-2.6.23-rc2-mm2/fs/autofs4/root.c.negative-timeout	2007-08-17 11:53:38.000000000 +0800
> > +++ linux-2.6.23-rc2-mm2/fs/autofs4/root.c	2007-08-21 15:44:34.000000000 +0800
> > @@ -238,6 +238,125 @@ out:
> >  	return dcache_readdir(file, dirent, filldir);
> >  }
> >  
> > +static int autofs4_compare_dentry(struct dentry *parent, struct dentry *dentry, struct qstr *name)
> > +{
> > +	unsigned int len = name->len;
> > +	unsigned int hash = name->hash;
> > +	const unsigned char *str = name->name;
> > +	struct qstr *qstr = &dentry->d_name;
> > +
> > +	if (dentry->d_name.hash != hash)
> > +		return 0;
> > +	if (dentry->d_parent != parent)
> > +		return 0;
> > +
> > +	if (qstr->len != len)
> > +		return 0;
> > +	if (memcmp(qstr->name, str, len))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> > +static struct dentry *autofs4_lookup_dentry(struct autofs_sb_info *sbi, struct dentry *dentry)
> > +{
> > +	struct dentry *parent = dentry->d_parent;
> > +	struct qstr *name = &dentry->d_name;
> > +	struct list_head *p, *head;
> > +
> > +	head = &sbi->rehash_list;
> > +	list_for_each(p, head) {
> > +		struct autofs_info *ino;
> > +		struct dentry *this;
> > +
> > +		ino = list_entry(p, struct autofs_info, rehash);
> > +		this = ino->dentry;
> > +
> > +		spin_lock(&this->d_lock);
> > +
> > +		if (!autofs4_compare_dentry(parent, this, name))
> > +			goto next;
> > +
> > +		if (d_unhashed(this)) {
> > +			spin_unlock(&this->d_lock);
> > +			return this;
> > +		}
> > +next:
> > +		spin_unlock(&this->d_lock);
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static struct dentry *autofs4_lookup_negative(struct autofs_sb_info *sbi, struct dentry *dentry)
> > +{
> > +	struct dentry *parent = dentry->d_parent;
> > +	struct qstr *name = &dentry->d_name;
> > +	struct list_head *p, *head;
> > +
> > +	spin_lock(&sbi->rehash_lock);
> > +	head = &sbi->rehash_list;
> > +	list_for_each(p, head) {
> > +		struct autofs_info *ino;
> > +		struct dentry *this;
> > +
> > +		ino = list_entry(p, struct autofs_info, rehash);
> > +		this = ino->dentry;
> > +
> > +		spin_lock(&this->d_lock);
> > +
> > +		if (this->d_inode)
> > +			goto next;
> > +
> > +		if (!autofs4_compare_dentry(parent, this, name))
> > +			goto next;
> > +
> > +		if (d_unhashed(this)) {
> > +			if (autofs4_oz_mode(sbi)) {
> > +				spin_unlock(&this->d_lock);
> > +				spin_unlock(&sbi->rehash_lock);
> > +				return this;
> > +			}
> > +			/*
> > +			 * If d_time is non-zero we've had a mount fail.
> > +			 * Check if it's time to forget about it, return
> > +			 * fail if not.
> > +			 */
> > +			if (this->d_time) {
> > +				struct autofs_info *p_ino;
> > +				unsigned long neg_timeout;
> > +
> > +				neg_timeout = this->d_time - get_seconds();
> > +				if (neg_timeout <= ino->negative_timeout) {
> > +					spin_unlock(&this->d_lock);
> > +					spin_unlock(&sbi->rehash_lock);
> > +					return ERR_PTR(-ENOENT);
> > +				}
> > +				/* Negative cache timeout */
> > +				this->d_time = 0;
> > +				list_del_init(&ino->rehash);
> > +				spin_unlock(&this->d_lock);
> > +				spin_unlock(&sbi->rehash_lock);
> > +				if (atomic_dec_and_test(&ino->count)) {
> > +					p_ino = autofs4_dentry_ino(this->d_parent);
> > +					if (p_ino && this->d_parent != this)
> > +						atomic_dec(&p_ino->count);
> > +				}
> > +				dput(this);
> > +				return NULL;
> > +			}
> > +			spin_unlock(&this->d_lock);
> > +			spin_unlock(&sbi->rehash_lock);
> > +			return this;
> > +		}
> > +next:
> > +		spin_unlock(&this->d_lock);
> > +	}
> > +	spin_unlock(&sbi->rehash_lock);
> > +
> > +	return NULL;
> > +}
> > +
> >  static int try_to_fill_dentry(struct dentry *dentry, int flags)
> >  {
> >  	struct autofs_sb_info *sbi = autofs4_sbi(dentry->d_sb);
> > @@ -282,7 +401,27 @@ static int try_to_fill_dentry(struct den
> >  
> >  		/* Turn this into a real negative dentry? */
> >  		if (status == -ENOENT) {
> > +			if (!ino) {
> > +				ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
> > +				if (ino == NULL)
> > +					return -ENOSPC;
> > +				dentry->d_fsdata = ino;
> > +			}
> > +			spin_lock(&sbi->rehash_lock);
> > +			if (!autofs4_lookup_dentry(sbi, dentry)) {
> > +				struct autofs_info *p_ino;
> > +				ino->dentry = dget(dentry);
> > +				atomic_inc(&ino->count);
> > +				p_ino = autofs4_dentry_ino(dentry->d_parent);
> > +				if (p_ino && dentry->d_parent != dentry)
> > +					atomic_inc(&p_ino->count);
> > +				list_add(&ino->rehash, &sbi->rehash_list);
> > +			}
> > +			spin_unlock(&sbi->rehash_lock);
> > +			/* Set time at which we forget about this mount fial */
> >  			spin_lock(&dentry->d_lock);
> > +			ino->negative_timeout = negative_timeout;
> > +			dentry->d_time = get_seconds() + negative_timeout;
> >  			dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> >  			spin_unlock(&dentry->d_lock);
> >  			return status;
> > @@ -305,6 +444,11 @@ static int try_to_fill_dentry(struct den
> >  
> >  		if (status) {
> >  			spin_lock(&dentry->d_lock);
> > +			/* Set time at which we forget about this mount fial */
> > +			if (status == -ENOENT) {
> > +				ino->negative_timeout = negative_timeout;
> > +				dentry->d_time = get_seconds() + negative_timeout;
> > +			}
> >  			dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> >  			spin_unlock(&dentry->d_lock);
> >  			return status;
> > @@ -316,6 +460,7 @@ static int try_to_fill_dentry(struct den
> >  		ino->last_used = jiffies;
> >  
> >  	spin_lock(&dentry->d_lock);
> > +	dentry->d_time = 0;
> >  	dentry->d_flags &= ~DCACHE_AUTOFS_PENDING;
> >  	spin_unlock(&dentry->d_lock);
> >  	return status;
> > @@ -339,6 +484,19 @@ static void *autofs4_follow_link(struct 
> >  	if (oz_mode || !lookup_type)
> >  		goto done;
> >  
> > +	/* Check for cached mount fail, reset d_time if timed out */
> > +	spin_lock(&dentry->d_lock);
> > +	if (dentry->d_time) {
> > +		unsigned long neg_timeout = dentry->d_time - get_seconds();
> > +		if (neg_timeout <= ino->negative_timeout) {
> > +			spin_unlock(&dentry->d_lock);
> > +			status = -ENOENT;
> > +			goto out_error;
> > +		}
> > +		dentry->d_time = 0;
> > +	}
> > +	spin_unlock(&dentry->d_lock);
> > +
> >  	/* If an expire request is pending wait for it. */
> >  	if (ino && (ino->flags & AUTOFS_INF_EXPIRING)) {
> >  		DPRINTK("waiting for active request %p name=%.*s",
> > @@ -426,9 +584,28 @@ static int autofs4_revalidate(struct den
> >  		return status;
> >  	}
> >  
> > -	/* Negative dentry.. invalidate if "old" */
> > -	if (dentry->d_inode == NULL)
> > -		return 0;
> > +	/* Check for cached mount fail, reset d_time if timed out */
> > +	spin_lock(&dentry->d_lock);
> > +	if (!dentry->d_inode || dentry->d_time) {
> > +		struct autofs_info *ino = autofs4_dentry_ino(dentry);
> > +		unsigned long neg_timeout = dentry->d_time - get_seconds();
> > +		/*
> > +		 * If d_time is non-zero we've had a mount fail. Check if
> > +		 * it's time to forget about it, return fail if not.
> > +		 */
> > +		if (dentry->d_time && (neg_timeout <= ino->negative_timeout)) {
> > +			spin_unlock(&dentry->d_lock);
> > +			return 1;
> > +		}
> > +		/* Negative timeout has expired */
> > +		dentry->d_time = 0;
> > +
> > +		if (!dentry->d_inode) {
> > +			spin_unlock(&dentry->d_lock);
> > +			return 0;
> > +		}
> > +	}
> > +	spin_unlock(&dentry->d_lock);
> >  
> >  	/* Check for a non-mountpoint directory with no contents */
> >  	spin_lock(&dcache_lock);
> > @@ -497,9 +674,6 @@ static struct dentry_operations autofs4_
> >  
> >  static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct dentry *parent, struct qstr *name)
> >  {
> > -	unsigned int len = name->len;
> > -	unsigned int hash = name->hash;
> > -	const unsigned char *str = name->name;
> >  	struct list_head *p, *head;
> >  
> >  	spin_lock(&dcache_lock);
> > @@ -508,7 +682,6 @@ static struct dentry *autofs4_lookup_unh
> >  	list_for_each(p, head) {
> >  		struct autofs_info *ino;
> >  		struct dentry *dentry;
> > -		struct qstr *qstr;
> >  
> >  		ino = list_entry(p, struct autofs_info, rehash);
> >  		dentry = ino->dentry;
> > @@ -519,20 +692,10 @@ static struct dentry *autofs4_lookup_unh
> >  		if (!dentry->d_inode)
> >  			goto next;
> >  
> > -		qstr = &dentry->d_name;
> > -
> > -		if (dentry->d_name.hash != hash)
> > -			goto next;
> > -		if (dentry->d_parent != parent)
> > -			goto next;
> > -
> > -		if (qstr->len != len)
> > -			goto next;
> > -		if (memcmp(qstr->name, str, len))
> > +		if (!autofs4_compare_dentry(parent, dentry, name))
> >  			goto next;
> >  
> >  		if (d_unhashed(dentry)) {
> > -			struct autofs_info *ino = autofs4_dentry_ino(dentry);
> >  			struct inode *inode = dentry->d_inode;
> >  
> >  			list_del_init(&ino->rehash);
> > @@ -568,7 +731,7 @@ next:
> >  static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
> >  {
> >  	struct autofs_sb_info *sbi;
> > -	struct dentry *unhashed;
> > +	struct dentry *unhashed, *negative = NULL;
> >  	int oz_mode;
> >  
> >  	DPRINTK("name = %.*s",
> > @@ -587,6 +750,22 @@ static struct dentry *autofs4_lookup(str
> >  	unhashed = autofs4_lookup_unhashed(sbi, dentry->d_parent, &dentry->d_name);
> >  	if (!unhashed) {
> >  		/*
> > +		 * Negative caching of failed "nobrowse" autofs mounts
> > +		 * require that we keep the unhashed, negative dentrys
> > +		 * around so we can use them to check failure status. We
> > +		 * use the rehash_list list in the super bloxk for this
> > +		 * purpose. This isn't needed for "browse" or direct mounts
> > +		 * as the dentrys are always positive so the info is always
> > +		 * available.
> > +		 */
> > +		negative = autofs4_lookup_negative(sbi, dentry);
> > +		if (negative) {
> > +			if (IS_ERR(negative))
> > +				return negative;
> > +			dentry = negative;
> > +			goto found;
> > +		}
> > +		/*
> >  		 * Mark the dentry incomplete but don't hash it. We do this 
> >  		 * to serialize our inode creation operations (symlink and
> >  		 * mkdir) which prevents deadlock during the callback to
> > @@ -599,6 +778,7 @@ static struct dentry *autofs4_lookup(str
> >  		 */
> >  		dentry->d_op = &autofs4_root_dentry_operations;
> >  
> > +		dentry->d_time = 0;
> >  		dentry->d_fsdata = NULL;
> >  		d_instantiate(dentry, NULL);
> >  	} else {
> > @@ -621,7 +801,7 @@ static struct dentry *autofs4_lookup(str
> >  		}
> >  		dentry = unhashed;
> >  	}
> > -
> > +found:
> >  	if (!oz_mode) {
> >  		spin_lock(&dentry->d_lock);
> >  		dentry->d_flags |= DCACHE_AUTOFS_PENDING;
> > @@ -682,7 +862,7 @@ static struct dentry *autofs4_lookup(str
> >  		return dentry;
> >  	}
> >  
> > -	if (unhashed)
> > +	if (unhashed || negative)
> >  		return dentry;
> >  
> >  	return NULL;
> > @@ -779,7 +959,9 @@ static int autofs4_dir_unlink(struct ino
> >  
> >  	spin_lock(&dcache_lock);
> >  	spin_lock(&sbi->rehash_lock);
> > -	list_add(&ino->rehash, &sbi->rehash_list);
> > +	dentry->d_time = 0;
> > +	if (!autofs4_lookup_dentry(sbi, dentry))
> > +		list_add(&ino->rehash, &sbi->rehash_list);
> >  	spin_unlock(&sbi->rehash_lock);
> >  	spin_lock(&dentry->d_lock);
> >  	__d_drop(dentry);
> > @@ -807,7 +989,9 @@ static int autofs4_dir_rmdir(struct inod
> >  		return -ENOTEMPTY;
> >  	}
> >  	spin_lock(&sbi->rehash_lock);
> > -	list_add(&ino->rehash, &sbi->rehash_list);
> > +	dentry->d_time = 0;
> > +	if (!autofs4_lookup_dentry(sbi, dentry))
> > +		list_add(&ino->rehash, &sbi->rehash_list);
> >  	spin_unlock(&sbi->rehash_lock);
> >  	spin_lock(&dentry->d_lock);
> >  	__d_drop(dentry);
> >
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >   
> 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/