Bug: devfs is sure to have the bug.

[Jaakko Heinonen]

On 2011-08-03, Kostik Belousov wrote:
> On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote:
> > > devfs_populate(), and the context holds only "dm->dm_lock" in
> > > devfs_populate().
> > > 
> > > On the other hand, "devfs_generation" is incremented in devfs_create()
> > > and devfs_destroy() the context holds only "devmtx" in devfs_create()
> > > and devfs_destroy().
> > > 
> > > If a context executes devfs_create() when other context is executing
> > > (***), then "dm->dm_generation" is updated incorrect value.
> > > As a result, we can not open the last detected device (we receive ENOENT).
> 
> I think the problem you described is real, and suggested change is right.
> Initially, I thought that we should work with devfs_generation as with
> the atomic type due to unlocked access in the devfs_populate(), but then
> convinced myself that this is not needed.
> 
> But also, I think there is another half of the problem. Namely,
> devfs_lookup() calls devfs_populate_vp(), and then does lookup with the
> help of devfs_lookupx(). We will miss the generation update
> happen after the drop of the dm_lock in devfs_populate_vp() to reacquire
> the directory vnode lock.

I don't understand this. devfs_generation is not protected with dm_lock
in devfs_create() and devfs_destroy(). On the other hand if you mean
that another thread calls devfs_populate() while we drop dm_lock in
devfs_populate_vp(), isn't the mount point up to date when we re-lock
dm_lock?

> @@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
>  void
>  devfs_populate(struct devfs_mount *dm)
>  {
> +	unsigned gen;
>  
>  	sx_assert(&dm->dm_lock, SX_XLOCKED);
> -	if (dm->dm_generation == devfs_generation)
> +	gen = devfs_generation;
> +	if (dm->dm_generation == gen)
>  		return;
>  	while (devfs_populate_loop(dm, 0))
>  		continue;
> -	dm->dm_generation = devfs_generation;
> +	dm->dm_generation = gen;
>  }

After this change dm->dm_generation may be stale although the mount
point is up to date? This is probably harmless, though.

-- 
Jaakko