Re: [PATCH] fs: open the block device after allocation the super_block

[Jan Kara <jack@suse.cz>]

On Wed 16-08-23 09:29:08, Christian Brauner wrote:
> On Tue, Aug 15, 2023 at 04:43:12PM +0200, Christian Brauner wrote:
> > >  		up_write(&s->s_umount);
> > > -		blkdev_put(bdev, fs_type);
> > > +		error = setup_bdev_super(s, flags, NULL);
> > >  		down_write(&s->s_umount);
> > 
> > So I've been looking through the branches to see what's ready for v6.6
> > and what needs some more time. While doing so I went over this again and
> > realized that we have an issue here.
> > 
> > While it looks like dropping s_umount here and calling
> > setup_bdev_super() is fine I think it isn't. Consider two processes
> > racing to create the same mount:
> > 
> > P1                                                                    P2
> > vfs_get_tree()                                                        vfs_get_tree()
> > -> get_tree() == get_tree_bdev()                                      -> get_tree() == get_tree_bdev()
> >    -> sget_fc()                                                          -> sget_fc()
> >         // allocate new sb; no matching sb found
> >       -> sb_p1 = alloc_super()
> >       -> hlist_add_head(&s->s_instances, &s->s_type->fs_supers)
> >       -> spin_unlock(&sb_lock)                                              
> >       // yield s_umount to avoid deadlocks
> >    -> up_write(&sb->s_umount)
> >                                                                             -> spin_lock(&sb_lock)
> >                                                                                // find sb_p1
> >                                                                                if (test(old, fc))
> >                                                                                        goto share_extant_sb;
> >       // Assume P1 sleeps on bdev_lock or open_mutex
> >       // in blkdev_get_by_dev().
> >    -> setup_bdev_super()
> >    -> down_write(&sb->s_umount)
> > 
> > Now P2 jumps to the share_extant_sb label and calls:
> > 
> > grab_super(sb_p1)
> > -> spin_unlock(&sb_lock)
> > -> down_write(&s->s_umount)
> > 
> > Since s_umount is unlocked P2 doesn't go to sleep and instead immediately
> > goes to retry by jumping to the "retry" label. If P1 is still sleeping
> > on a a bdev mutex the same thing happens again.
> > 
> > So if you have a range of processes P{1,n} that all try to mount the
> > same device you're hammering endlessly on sb_lock without ever going to
> > sleep like we used to. The same problem exists for all iterate_supers()
> 
> That part is wrong. If you have P{1,n} and P1 takes s_umount exclusively
> then P{2,n} will sleep on s_umount until P1 is done. But there's still
> at least on process spinning through sget_fc() for no good reason.

No, you're right that the second process is going to effectively busyloop
waiting for SB_BORN to be set. I agree we should add some sleeping wait to
the loop to avoid pointlessly burning CPU cycles. I'll look into some
elegant solution tomorrow.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR