| Message ID | 1569813459-8601-1-git-send-email-sunny.s.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: move the mlog from the spinlock to outside | expand |
On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote: > There is a potential task of deadlock. Because the mask > is 0, the deadlock does not occur now. But There is a > potential task. If someone change the mask of mlog, but > forget to modify the order of the mlog and spin_unlock, > There will be a potential deadlock.So I move the mlog > from the spinlock to outsize. > > ... > > --- a/fs/ocfs2/dlmglue.c > +++ b/fs/ocfs2/dlmglue.c > @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, > > spin_lock(&oi->ip_lock); > if (oi->ip_flags & OCFS2_INODE_DELETED) { > + spin_unlock(&oi->ip_lock); > mlog(0, "Orphaned inode %llu was deleted while we " > "were waiting on a lock. ip_flags = 0x%x\n", > (unsigned long long)oi->ip_blkno, oi->ip_flags); > - spin_unlock(&oi->ip_lock); > status = -ENOENT; > goto bail; > } The patch is obviously OK but I don't see any deadlock. mlog() doesn't take any locks?
On 19/10/8 06:11, Andrew Morton wrote: > On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote: > >> There is a potential task of deadlock. Because the mask >> is 0, the deadlock does not occur now. But There is a >> potential task. If someone change the mask of mlog, but >> forget to modify the order of the mlog and spin_unlock, >> There will be a potential deadlock.So I move the mlog >> from the spinlock to outsize. >> >> ... >> >> --- a/fs/ocfs2/dlmglue.c >> +++ b/fs/ocfs2/dlmglue.c >> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, >> >> spin_lock(&oi->ip_lock); >> if (oi->ip_flags & OCFS2_INODE_DELETED) { >> + spin_unlock(&oi->ip_lock); >> mlog(0, "Orphaned inode %llu was deleted while we " >> "were waiting on a lock. ip_flags = 0x%x\n", >> (unsigned long long)oi->ip_blkno, oi->ip_flags); >> - spin_unlock(&oi->ip_lock); >> status = -ENOENT; >> goto bail; >> } > > The patch is obviously OK but I don't see any deadlock. mlog() doesn't > take any locks? > I guess Shuning refers the calling of printk with spin lock. > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel >
在 2019/10/8 上午9:14, Joseph Qi 写道: > > On 19/10/8 06:11, Andrew Morton wrote: >> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote: >> >>> There is a potential task of deadlock. Because the mask >>> is 0, the deadlock does not occur now. But There is a >>> potential task. If someone change the mask of mlog, but >>> forget to modify the order of the mlog and spin_unlock, >>> There will be a potential deadlock.So I move the mlog >>> from the spinlock to outsize. >>> >>> ... >>> >>> --- a/fs/ocfs2/dlmglue.c >>> +++ b/fs/ocfs2/dlmglue.c >>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, >>> >>> spin_lock(&oi->ip_lock); >>> if (oi->ip_flags & OCFS2_INODE_DELETED) { >>> + spin_unlock(&oi->ip_lock); >>> mlog(0, "Orphaned inode %llu was deleted while we " >>> "were waiting on a lock. ip_flags = 0x%x\n", >>> (unsigned long long)oi->ip_blkno, oi->ip_flags); >>> - spin_unlock(&oi->ip_lock); >>> status = -ENOENT; >>> goto bail; >>> } >> The patch is obviously OK but I don't see any deadlock. mlog() doesn't >> take any locks? >> > I guess Shuning refers the calling of printk with spin lock. Yes, It is the calling of printk. Sorry for the description is not clear enough. :) >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >>
On 19/10/8 13:00, sunnyZhang wrote: > > 在 2019/10/8 上午9:14, Joseph Qi 写道: >> >> On 19/10/8 06:11, Andrew Morton wrote: >>> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote: >>> >>>> There is a potential task of deadlock. Because the mask >>>> is 0, the deadlock does not occur now. But There is a >>>> potential task. If someone change the mask of mlog, but >>>> forget to modify the order of the mlog and spin_unlock, >>>> There will be a potential deadlock.So I move the mlog >>>> from the spinlock to outsize. >>>> >>>> ... >>>> >>>> --- a/fs/ocfs2/dlmglue.c >>>> +++ b/fs/ocfs2/dlmglue.c >>>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, >>>> spin_lock(&oi->ip_lock); >>>> if (oi->ip_flags & OCFS2_INODE_DELETED) { >>>> + spin_unlock(&oi->ip_lock); >>>> mlog(0, "Orphaned inode %llu was deleted while we " >>>> "were waiting on a lock. ip_flags = 0x%x\n", >>>> (unsigned long long)oi->ip_blkno, oi->ip_flags); >>>> - spin_unlock(&oi->ip_lock); >>>> status = -ENOENT; >>>> goto bail; >>>> } >>> The patch is obviously OK but I don't see any deadlock. mlog() doesn't >>> take any locks? >>> >> I guess Shuning refers the calling of printk with spin lock. > > Yes, It is the calling of printk. > > Sorry for the description is not clear enough. :) > IIUC, we can call printk with spinlock, no deadlock will happen. So I have the same question, where is the "potential deadlock"? Thanks, Joseph
在 2019/10/8 下午5:51, Joseph Qi 写道: > > On 19/10/8 13:00, sunnyZhang wrote: >> 在 2019/10/8 上午9:14, Joseph Qi 写道: >>> On 19/10/8 06:11, Andrew Morton wrote: >>>> On Mon, 30 Sep 2019 11:17:39 +0800 Shuning Zhang <sunny.s.zhang@oracle.com> wrote: >>>> >>>>> There is a potential task of deadlock. Because the mask >>>>> is 0, the deadlock does not occur now. But There is a >>>>> potential task. If someone change the mask of mlog, but >>>>> forget to modify the order of the mlog and spin_unlock, >>>>> There will be a potential deadlock.So I move the mlog >>>>> from the spinlock to outsize. >>>>> >>>>> ... >>>>> >>>>> --- a/fs/ocfs2/dlmglue.c >>>>> +++ b/fs/ocfs2/dlmglue.c >>>>> @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, >>>>> spin_lock(&oi->ip_lock); >>>>> if (oi->ip_flags & OCFS2_INODE_DELETED) { >>>>> + spin_unlock(&oi->ip_lock); >>>>> mlog(0, "Orphaned inode %llu was deleted while we " >>>>> "were waiting on a lock. ip_flags = 0x%x\n", >>>>> (unsigned long long)oi->ip_blkno, oi->ip_flags); >>>>> - spin_unlock(&oi->ip_lock); >>>>> status = -ENOENT; >>>>> goto bail; >>>>> } >>>> The patch is obviously OK but I don't see any deadlock. mlog() doesn't >>>> take any locks? >>>> >>> I guess Shuning refers the calling of printk with spin lock. >> Yes, It is the calling of printk. >> >> Sorry for the description is not clear enough. :) >> > IIUC, we can call printk with spinlock, no deadlock will happen. > So I have the same question, where is the "potential deadlock"? I apology for my stupidity. I have always had a wrong memory, I think printk() can sleep. @Andrew Please can you help me to withdraw this patch? Thanks > Thanks, > Joseph >
diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c index 1420723..9960e61 100644 --- a/fs/ocfs2/dlmglue.c +++ b/fs/ocfs2/dlmglue.c @@ -2315,10 +2315,10 @@ static int ocfs2_inode_lock_update(struct inode *inode, spin_lock(&oi->ip_lock); if (oi->ip_flags & OCFS2_INODE_DELETED) { + spin_unlock(&oi->ip_lock); mlog(0, "Orphaned inode %llu was deleted while we " "were waiting on a lock. ip_flags = 0x%x\n", (unsigned long long)oi->ip_blkno, oi->ip_flags); - spin_unlock(&oi->ip_lock); status = -ENOENT; goto bail; }
There is a potential task of deadlock. Because the mask is 0, the deadlock does not occur now. But There is a potential task. If someone change the mask of mlog, but forget to modify the order of the mlog and spin_unlock, There will be a potential deadlock.So I move the mlog from the spinlock to outsize. Signed-off-by: Shuning Zhang <sunny.s.zhang@oracle.com> --- fs/ocfs2/dlmglue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)