Re: [PATCH] ocfs2: kill osb->system_file_mutex lock

[Heming Zhao]

On Sat, May 16, 2026 at 10:10:44PM +0900, Tetsuo Handa wrote:
> On 2026/05/16 21:27, Heming Zhao wrote:
> >>>> In my opinion, the problem with the current code is that the scope of
> >>>> mutex_lock(&osb->system_file_mutex) is too broad. This mutex only needs to be
> >>>> held prior to calling _ocfs2_get_system_file_inode(). I previously highlighted
> >>>> this point in my initial review comment on the patch.
> >>
> >> If you meant
> >>
> >>  struct inode *ocfs2_get_system_file_inode(struct ocfs2_super *osb,
> >>                                           int type,
> >>                                           u32 slot)
> >>  {
> >>         struct inode *inode = NULL;
> >>         struct inode **arr = NULL;
> >>
> >>         /* avoid the lookup if cached in local system file array */
> >>         if (is_global_system_inode(type)) {
> >>                 arr = &(osb->global_system_inodes[type]);
> >>         } else
> >>                 arr = get_local_system_inode(osb, type, slot);
> >>
> >> -       mutex_lock(&osb->system_file_mutex);
> >>         if (arr && ((inode = *arr) != NULL)) {
> >>                 /* get a ref in addition to the array ref */
> >>                 inode = igrab(inode);
> >>                 mutex_unlock(&osb->system_file_mutex);
> >>                 BUG_ON(!inode);
> >>
> >>                 return inode;
> >>         }
> >>
> >> +       mutex_lock(&osb->system_file_mutex);
> >> +
> >>         /* this gets one ref thru iget */
> >>         inode = _ocfs2_get_system_file_inode(osb, type, slot);
> >>
> >>         /* add one more if putting into array for first time */
> >>         if (arr && inode) {
> >>                 *arr = igrab(inode);
> >>                 BUG_ON(!*arr);
> >>         }
> >>         mutex_unlock(&osb->system_file_mutex);
> >>         return inode;
> >>  }
> >>
> > 
> > Yes, this is what I expected.
> > 
> >> , that brings us back to before commit 43b10a20372d, for two threads can hit
> >> the "*arr = igrab(inode)" line.
> > 
> > I don't think this bring us back, because igrab() uses a spinlock to protect the
> > refcount.
> 
> You are missing the point. What is wrong with above change is that two threads can
> sequentially enter into the "if (arr && inode) { }" block, for we are not checking
> whether the slot is already NULL or not. The cmpxchg() is the magic that guarantees
> that only one thread can enter into that block.

To avoid my code issue, the code should be:

     mutex_lock(&osb->system_file_mutex);

     if (arr && !*arr) {
         /* this gets one ref thru iget */
         inode = _ocfs2_get_system_file_inode(osb, type, slot);

         /* add one more if putting into array for first time */
         if (inode) {
             *arr = igrab(inode);
             BUG_ON(!*arr);
         }
     } else if (arr && *arr) {
	 /* get a ref in addition to the array ref */
	 inode = igrab(*arr);
	 BUG_ON(!inode);
     }
     mutex_unlock(&osb->system_file_mutex);

If apply this change, the code will look too complicated and hard to maintain.
Regarding the removal of the mutex, your patch looks good to me now.

My new comment for your patch:
Because this issue was triggered by syzbot testing Diogo Jahchan Koike
<djahchankoike@xxxxxxxxx>'s patch for bug[1], I think it's better to removed the
'Reported-by' and close' tags, and describe the history in commit log. 
Current 'Reported-by' & 'close' tags are misleading.

[1]: https://syzkaller.appspot.com/bug?extid=1fed2de07d8e11a3ec1b

Thanks,
Heming
> 
> >>
> >> Even if you also change the "if (arr && inode) {" check in order to make sure that
> >> the second-reached thread won't again hit the "*arr = igrab(inode)" line when the
> >> first-reached thread already hit the "*arr = igrab(inode)" line, the second-reached
> >> thread is fully blocked by the first-reached thread. Not serializing two threads
> >> allows these threads to return as quick as serializing these threads.
> > 
> > with the mutex protection removed, and relying on igrab()'s internal spinlock,
> > the code logic is already correct for concurrency. 
> > 
> > there are two kinds of locks involved in this discussion: 
> > - inode->i_lock: protects the inode's refcount (used by igrab())
> > - osb->system_file_mutex: protects the array slot.
> 
> You are missing the point. The lock used inside igrab() is irrelevant.
> The cmpxchg() is the lock that protects the array slot (in other words,
> serializes two threads) without using the system_file_mutex lock.
>