Messages in this thread | | | Date | Thu, 12 Jun 2008 12:13:48 -0700 | From | Joel Becker <> | Subject | Re: [PATCH 1/3][BUGFIX] configfs: Introduce configfs_dirent_lock |
| |
On Thu, Jun 12, 2008 at 03:31:27PM +0200, Louis Rilling wrote: > This patch introduces configfs_dirent_lock spinlock to protect configfs_dirent > traversals against linkage mutations (add/del/move). This will allow > configfs_detach_prep() to avoid locking i_mutexes.
I like that you expanded the scope to cover all configfs_dirent linkages. These are all protected by i_mutex in the current code, but your rename fix removes that protection.
> Locking rules for configfs_dirent linkage mutations are the same plus the > requirement of taking configfs_dirent_lock. For configfs_dirent walking, one can > either take appropriate i_mutex as before, or take configfs_dirent_lock.
Nope, you *must* take configfs_dirent_lock now. You've removed i_mutex protection in the last patch.
> The spinlock could actually be a mutex, but the critical sections are either > O(1) or should not be too long (default groups walking in last patch).
I'm wary of someone's reasonably deep groups. Discussing it yesterday I'd been convinced that a mutex was good to start with. But given your increased scope of the lock, let's try the spinlock and see what happens.
> +extern spinlock_t configfs_dirent_lock;
Boy I wish this could be static to one file :-(
> @@ -79,7 +84,9 @@ static struct configfs_dirent *configfs_ > atomic_set(&sd->s_count, 1); > INIT_LIST_HEAD(&sd->s_links); > INIT_LIST_HEAD(&sd->s_children); > + spin_lock(&configfs_dirent_lock); > list_add(&sd->s_sibling, &parent_sd->s_children); > + spin_unlock(&configfs_dirent_lock); > sd->s_element = element;
You need to set s_element either under the lock or before taking the lock. Once you've dropped the lock, someone can reach this dirent from the parent, and should see s_element.
> @@ -173,7 +180,9 @@ static int create_dir(struct config_item > } else { > struct configfs_dirent *sd = d->d_fsdata; > if (sd) { > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_put(sd); > } > } > @@ -224,7 +233,9 @@ int configfs_create_link(struct configfs > else { > struct configfs_dirent *sd = dentry->d_fsdata; > if (sd) { > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_put(sd); > } > }
These strike me as wrong - I would think that one should either see the old configfs_dirent tree or the new one. That is, one would take the dirent lock at the beginning of configfs_mkdir() and release it at the end - so any other code that looks at the dirent tree will see an atomic change. Here, some other path could see the new dirent after configfs_make_dirent() but then it disappears when configfs_create() fails. If you did that, though, it'd have to be a mutex. Now, the only thing that sees this intermediate condition is configfs itself. Everyone else is protected by i_mutex. I guess it's OK - but can you comment that fact? i_mutex does *not* protect traversal of the configfs_dirent tree, but it does prevent the outside world from seeing the intermediate states.
> @@ -238,7 +249,9 @@ static void remove_dir(struct dentry * d > struct configfs_dirent * sd; > > sd = d->d_fsdata; > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_put(sd); > if (d->d_inode) > simple_rmdir(parent->d_inode,d); > @@ -410,7 +423,9 @@ static void detach_attrs(struct config_i > list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) { > if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED)) > continue; > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_drop_dentry(sd, dentry); > configfs_put(sd); > } > @@ -1268,7 +1283,9 @@ static int configfs_dir_close(struct ino > struct configfs_dirent * cursor = file->private_data; > > mutex_lock(&dentry->d_inode->i_mutex); > + spin_lock(&configfs_dirent_lock); > list_del_init(&cursor->s_sibling); > + spin_unlock(&configfs_dirent_lock); > mutex_unlock(&dentry->d_inode->i_mutex); > > release_configfs_dirent(cursor); > @@ -1362,7 +1383,9 @@ static loff_t configfs_dir_lseek(struct > struct list_head *p; > loff_t n = file->f_pos - 2; > > + spin_lock(&configfs_dirent_lock); > list_del(&cursor->s_sibling); > + spin_unlock(&configfs_dirent_lock); > p = sd->s_children.next; > while (n && p != &sd->s_children) { > struct configfs_dirent *next; > @@ -1372,7 +1395,9 @@ static loff_t configfs_dir_lseek(struct > n--; > p = p->next; > } > + spin_lock(&configfs_dirent_lock); > list_add_tail(&cursor->s_sibling, p); > + spin_unlock(&configfs_dirent_lock); > } > } > mutex_unlock(&dentry->d_inode->i_mutex); > Index: b/fs/configfs/inode.c > =================================================================== > --- a/fs/configfs/inode.c 2008-06-12 13:44:27.000000000 +0200 > +++ b/fs/configfs/inode.c 2008-06-12 13:44:34.000000000 +0200 > @@ -247,7 +247,9 @@ void configfs_hash_and_remove(struct den > if (!sd->s_element) > continue; > if (!strcmp(configfs_get_name(sd), name)) { > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_drop_dentry(sd, dir); > configfs_put(sd); > break; > Index: b/fs/configfs/symlink.c > =================================================================== > --- a/fs/configfs/symlink.c 2008-06-12 13:44:27.000000000 +0200 > +++ b/fs/configfs/symlink.c 2008-06-12 13:44:34.000000000 +0200 > @@ -169,7 +169,9 @@ int configfs_unlink(struct inode *dir, s > parent_item = configfs_get_config_item(dentry->d_parent); > type = parent_item->ci_type; > > + spin_lock(&configfs_dirent_lock); > list_del_init(&sd->s_sibling); > + spin_unlock(&configfs_dirent_lock); > configfs_drop_dentry(sd, dentry->d_parent); > dput(dentry); > configfs_put(sd);
You do the lock,del(sibling),unlock so often, maybe it should be a helper. Then you can make configfs_dirent_lock static to dir.c. Well, you use dirent_lock in your s_links patch, so maybe not static.
Joel
--
"Copy from one, it's plagiarism; copy from two, it's research." - Wilson Mizner
Joel Becker Principal Software Developer Oracle E-mail: joel.becker@oracle.com Phone: (650) 506-8127
| |