* [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-02-28 10:17 Larry Chen 2018-03-01 0:36 ` Changwei Ge 2018-03-01 12:56 ` piaojun 0 siblings, 2 replies; 15+ messages in thread From: Larry Chen @ 2018-02-28 10:17 UTC (permalink / raw) To: ocfs2-devel The function ocfs2_double_lock tries to lock the inode with lower blockid first, not lockid. Signed-off-by: Larry Chen <lchen@suse.com> --- fs/ocfs2/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c index c801eddc4bf3..30d454de35a8 100644 --- a/fs/ocfs2/namei.c +++ b/fs/ocfs2/namei.c @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, if (*bh2) *bh2 = NULL; - /* we always want to lock the one with the lower lockid first. + /* we always want to lock the one with the lower blockid first. * and if they are nested, we lock ancestor first */ if (oi1->ip_blkno != oi2->ip_blkno) { inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, -- 2.13.6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-02-28 10:17 [Ocfs2-devel] [PATCH] Correct a comment error Larry Chen @ 2018-03-01 0:36 ` Changwei Ge 2018-03-01 12:56 ` piaojun 1 sibling, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-01 0:36 UTC (permalink / raw) To: Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Larry, On 2018/2/28 18:18, Larry Chen wrote: > The function ocfs2_double_lock tries to lock the inode with lower > blockid first, not lockid. As ocfs2's lock name includes block number, so I think the comment you want to rework is all right. So nack. Thanks, Changwei > > Signed-off-by: Larry Chen <lchen@suse.com> > --- > fs/ocfs2/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index c801eddc4bf3..30d454de35a8 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > if (*bh2) > *bh2 = NULL; > > - /* we always want to lock the one with the lower lockid first. > + /* we always want to lock the one with the lower blockid first. > * and if they are nested, we lock ancestor first */ > if (oi1->ip_blkno != oi2->ip_blkno) { > inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-01 0:36 ` Changwei Ge 0 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-01 0:36 UTC (permalink / raw) To: Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Larry, On 2018/2/28 18:18, Larry Chen wrote: > The function ocfs2_double_lock tries to lock the inode with lower > blockid first, not lockid. As ocfs2's lock name includes block number, so I think the comment you want to rework is all right. So nack. Thanks, Changwei > > Signed-off-by: Larry Chen <lchen@suse.com> > --- > fs/ocfs2/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index c801eddc4bf3..30d454de35a8 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > if (*bh2) > *bh2 = NULL; > > - /* we always want to lock the one with the lower lockid first. > + /* we always want to lock the one with the lower blockid first. > * and if they are nested, we lock ancestor first */ > if (oi1->ip_blkno != oi2->ip_blkno) { > inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] 答复： Re: [PATCH] Correct a comment error 2018-03-01 0:36 ` Changwei Ge (?) @ 2018-03-01 6:59 ` Lei Chen 2018-03-02 2:00 ` Changwei Ge -1 siblings, 1 reply; 15+ messages in thread From: Lei Chen @ 2018-03-01 6:59 UTC (permalink / raw) To: ocfs2-devel Hi Changwei, Thanks for your review. I think the original comment is not as clear as you said since the code does not reflect any relation between lockid and blockid. Besides, the function inside indeed uses the block number for comparision. And it's really misleading for new beginners ,emmmm, like me. :) Thanks, Larry >>> Changwei Ge <ge.changwei@h3c.com> 2018-3-1 ?? 8:37 >>> Hi Larry, On 2018/2/28 18:18, Larry Chen wrote: > The function ocfs2_double_lock tries to lock the inode with lower > blockid first, not lockid. As ocfs2's lock name includes block number, so I think the comment you want to rework is all right. So nack. Thanks, Changwei > > Signed-off-by: Larry Chen <lchen@suse.com> > --- > fs/ocfs2/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index c801eddc4bf3..30d454de35a8 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > if (*bh2) > *bh2 = NULL; > > - /* we always want to lock the one with the lower lockid first. > + /* we always want to lock the one with the lower blockid first. > * and if they are nested, we lock ancestor first */ > if (oi1->ip_blkno != oi2->ip_blkno) { > inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > -------------- next part -------------- An HTML attachment was scrubbed... URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20180228/db74a027/attachment.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] 答复： Re: [PATCH] Correct a comment error 2018-03-01 6:59 ` [Ocfs2-devel] 答复： " Lei Chen @ 2018-03-02 2:00 ` Changwei Ge 0 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-02 2:00 UTC (permalink / raw) To: ocfs2-devel On 2018/3/1 15:00, Lei Chen wrote: > Hi Changwei, > > Thanks for your review. > > I think the original comment is not as clear as you said since > the code does not reflect any relation between lockid and blockid. > > Besides, the function inside indeed uses the block number for > comparision. And it's really misleading for new beginners ,emmmm, like me. It's OK. Any behavior to fix, improve ocfs2 is encouraged. -Changwei > > :) > > Thanks, > Larry > > > > >>>> Changwei?Ge?<ge.changwei@h3c.com> 2018-3-1 ?? 8:37 >>> > Hi Larry, > > On 2018/2/28 18:18, Larry Chen wrote: > > The function ocfs2_double_lock tries to lock the inode with lower > > blockid first, not lockid. > > As ocfs2's lock name includes block number, so I think the comment you want to > rework is all right. > So nack. > > Thanks, > Changwei > > > > > Signed-off-by: Larry Chen <lchen@suse.com> > > --- > > fs/ocfs2/namei.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > > index c801eddc4bf3..30d454de35a8 100644 > > --- a/fs/ocfs2/namei.c > > +++ b/fs/ocfs2/namei.c > > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > > ????if (*bh2) > > ????????*bh2 = NULL; > > > > -????/* we always want to lock the one with the lower lockid first. > > +????/* we always want to lock the one with the lower blockid first. > > ???? * and if they are nested, we lock ancestor first */ > > ????if (oi1->ip_blkno != oi2->ip_blkno) { > > ????????inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-02-28 10:17 [Ocfs2-devel] [PATCH] Correct a comment error Larry Chen @ 2018-03-01 12:56 ` piaojun 2018-03-01 12:56 ` piaojun 1 sibling, 0 replies; 15+ messages in thread From: piaojun @ 2018-03-01 12:56 UTC (permalink / raw) To: Larry Chen, mfasheh, jlbec; +Cc: linux-kernel, ocfs2-devel Hi Larry, There is the same mistake in ocfs2_reflink_inodes_lock(), could you help fixing them all? thanks, Jun On 2018/2/28 18:17, Larry Chen wrote: > The function ocfs2_double_lock tries to lock the inode with lower > blockid first, not lockid. > > Signed-off-by: Larry Chen <lchen@suse.com> > --- > fs/ocfs2/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index c801eddc4bf3..30d454de35a8 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > if (*bh2) > *bh2 = NULL; > > - /* we always want to lock the one with the lower lockid first. > + /* we always want to lock the one with the lower blockid first. > * and if they are nested, we lock ancestor first */ > if (oi1->ip_blkno != oi2->ip_blkno) { > inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-01 12:56 ` piaojun 0 siblings, 0 replies; 15+ messages in thread From: piaojun @ 2018-03-01 12:56 UTC (permalink / raw) To: Larry Chen, mfasheh, jlbec; +Cc: linux-kernel, ocfs2-devel Hi Larry, There is the same mistake in ocfs2_reflink_inodes_lock(), could you help fixing them all? thanks, Jun On 2018/2/28 18:17, Larry Chen wrote: > The function ocfs2_double_lock tries to lock the inode with lower > blockid first, not lockid. > > Signed-off-by: Larry Chen <lchen@suse.com> > --- > fs/ocfs2/namei.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c > index c801eddc4bf3..30d454de35a8 100644 > --- a/fs/ocfs2/namei.c > +++ b/fs/ocfs2/namei.c > @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, > if (*bh2) > *bh2 = NULL; > > - /* we always want to lock the one with the lower lockid first. > + /* we always want to lock the one with the lower blockid first. > * and if they are nested, we lock ancestor first */ > if (oi1->ip_blkno != oi2->ip_blkno) { > inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-03-01 12:56 ` piaojun @ 2018-03-02 1:59 ` Changwei Ge -1 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-02 1:59 UTC (permalink / raw) To: piaojun, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Jun, I think the comments for both two functions are OK. No need to rework them. As we know, ocfs2 lock name(lock id) are composed of several parts including block number. Thanks, Changw2ei On 2018/3/1 20:58, piaojun wrote: > Hi Larry, > > There is the same mistake in ocfs2_reflink_inodes_lock(), could you help > fixing them all? > > thanks, > Jun > > On 2018/2/28 18:17, Larry Chen wrote: >> The function ocfs2_double_lock tries to lock the inode with lower >> blockid first, not lockid. >> >> Signed-off-by: Larry Chen <lchen@suse.com> >> --- >> fs/ocfs2/namei.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >> index c801eddc4bf3..30d454de35a8 100644 >> --- a/fs/ocfs2/namei.c >> +++ b/fs/ocfs2/namei.c >> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >> if (*bh2) >> *bh2 = NULL; >> >> - /* we always want to lock the one with the lower lockid first. >> + /* we always want to lock the one with the lower blockid first. >> * and if they are nested, we lock ancestor first */ >> if (oi1->ip_blkno != oi2->ip_blkno) { >> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >> > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel at oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-02 1:59 ` Changwei Ge 0 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-02 1:59 UTC (permalink / raw) To: piaojun, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Jun, I think the comments for both two functions are OK. No need to rework them. As we know, ocfs2 lock name(lock id) are composed of several parts including block number. Thanks, Changw2ei On 2018/3/1 20:58, piaojun wrote: > Hi Larry, > > There is the same mistake in ocfs2_reflink_inodes_lock(), could you help > fixing them all? > > thanks, > Jun > > On 2018/2/28 18:17, Larry Chen wrote: >> The function ocfs2_double_lock tries to lock the inode with lower >> blockid first, not lockid. >> >> Signed-off-by: Larry Chen <lchen@suse.com> >> --- >> fs/ocfs2/namei.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >> index c801eddc4bf3..30d454de35a8 100644 >> --- a/fs/ocfs2/namei.c >> +++ b/fs/ocfs2/namei.c >> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >> if (*bh2) >> *bh2 = NULL; >> >> - /* we always want to lock the one with the lower lockid first. >> + /* we always want to lock the one with the lower blockid first. >> * and if they are nested, we lock ancestor first */ >> if (oi1->ip_blkno != oi2->ip_blkno) { >> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >> > > _______________________________________________ > Ocfs2-devel mailing list > Ocfs2-devel@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-03-02 1:59 ` Changwei Ge @ 2018-03-02 2:15 ` piaojun -1 siblings, 0 replies; 15+ messages in thread From: piaojun @ 2018-03-02 2:15 UTC (permalink / raw) To: Changwei Ge, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Changwei, On 2018/3/2 9:59, Changwei Ge wrote: > Hi Jun, > I think the comments for both two functions are OK. > No need to rework them. > As we know, ocfs2 lock name(lock id) are composed of several parts including > block number. I looked though the comments involved 'lockid', and found 'lockid' is a concept in dlm level, so ocfs2 level should not be aware of it. thanks, Jun > > Thanks, > Changw2ei > > On 2018/3/1 20:58, piaojun wrote: >> Hi Larry, >> >> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >> fixing them all? >> >> thanks, >> Jun >> >> On 2018/2/28 18:17, Larry Chen wrote: >>> The function ocfs2_double_lock tries to lock the inode with lower >>> blockid first, not lockid. >>> >>> Signed-off-by: Larry Chen <lchen@suse.com> >>> --- >>> fs/ocfs2/namei.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>> index c801eddc4bf3..30d454de35a8 100644 >>> --- a/fs/ocfs2/namei.c >>> +++ b/fs/ocfs2/namei.c >>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>> if (*bh2) >>> *bh2 = NULL; >>> >>> - /* we always want to lock the one with the lower lockid first. >>> + /* we always want to lock the one with the lower blockid first. >>> * and if they are nested, we lock ancestor first */ >>> if (oi1->ip_blkno != oi2->ip_blkno) { >>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>> >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel at oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> > . > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-02 2:15 ` piaojun 0 siblings, 0 replies; 15+ messages in thread From: piaojun @ 2018-03-02 2:15 UTC (permalink / raw) To: Changwei Ge, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Changwei, On 2018/3/2 9:59, Changwei Ge wrote: > Hi Jun, > I think the comments for both two functions are OK. > No need to rework them. > As we know, ocfs2 lock name(lock id) are composed of several parts including > block number. I looked though the comments involved 'lockid', and found 'lockid' is a concept in dlm level, so ocfs2 level should not be aware of it. thanks, Jun > > Thanks, > Changw2ei > > On 2018/3/1 20:58, piaojun wrote: >> Hi Larry, >> >> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >> fixing them all? >> >> thanks, >> Jun >> >> On 2018/2/28 18:17, Larry Chen wrote: >>> The function ocfs2_double_lock tries to lock the inode with lower >>> blockid first, not lockid. >>> >>> Signed-off-by: Larry Chen <lchen@suse.com> >>> --- >>> fs/ocfs2/namei.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>> index c801eddc4bf3..30d454de35a8 100644 >>> --- a/fs/ocfs2/namei.c >>> +++ b/fs/ocfs2/namei.c >>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>> if (*bh2) >>> *bh2 = NULL; >>> >>> - /* we always want to lock the one with the lower lockid first. >>> + /* we always want to lock the one with the lower blockid first. >>> * and if they are nested, we lock ancestor first */ >>> if (oi1->ip_blkno != oi2->ip_blkno) { >>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>> >> >> _______________________________________________ >> Ocfs2-devel mailing list >> Ocfs2-devel@oss.oracle.com >> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >> > . > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-03-02 2:15 ` piaojun @ 2018-03-02 2:37 ` Larry Chen -1 siblings, 0 replies; 15+ messages in thread From: Larry Chen @ 2018-03-02 2:37 UTC (permalink / raw) To: piaojun, Changwei Ge, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Changwei and Jun, Thanks for your advice. On 03/02/2018 10:15 AM, piaojun wrote: > Hi Changwei, > > On 2018/3/2 9:59, Changwei Ge wrote: >> Hi Jun, >> I think the comments for both two functions are OK. >> No need to rework them. >> As we know, ocfs2 lock name(lock id) are composed of several parts including >> block number. > I looked though the comments involved 'lockid', and found 'lockid' is a > concept in dlm level, so ocfs2 level should not be aware of it. When reading source code of ocfs2 level, you'll find that 'lockid' is a new concept that has not occurred or be explained before. It's kind of difficult to understand. Maybe 'blockno' or 'blockid' is more sensible than 'lockid' I'm willing to fix them all. Thanks Larry > thanks, > Jun >> Thanks, >> Changw2ei >> >> On 2018/3/1 20:58, piaojun wrote: >>> Hi Larry, >>> >>> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >>> fixing them all? >>> >>> thanks, >>> Jun >>> >>> On 2018/2/28 18:17, Larry Chen wrote: >>>> The function ocfs2_double_lock tries to lock the inode with lower >>>> blockid first, not lockid. >>>> >>>> Signed-off-by: Larry Chen <lchen@suse.com> >>>> --- >>>> fs/ocfs2/namei.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>>> index c801eddc4bf3..30d454de35a8 100644 >>>> --- a/fs/ocfs2/namei.c >>>> +++ b/fs/ocfs2/namei.c >>>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>>> if (*bh2) >>>> *bh2 = NULL; >>>> >>>> - /* we always want to lock the one with the lower lockid first. >>>> + /* we always want to lock the one with the lower blockid first. >>>> * and if they are nested, we lock ancestor first */ >>>> if (oi1->ip_blkno != oi2->ip_blkno) { >>>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>>> >>> _______________________________________________ >>> Ocfs2-devel mailing list >>> Ocfs2-devel at oss.oracle.com >>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >>> >> . >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-02 2:37 ` Larry Chen 0 siblings, 0 replies; 15+ messages in thread From: Larry Chen @ 2018-03-02 2:37 UTC (permalink / raw) To: piaojun, Changwei Ge, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Changwei and Jun, Thanks for your advice. On 03/02/2018 10:15 AM, piaojun wrote: > Hi Changwei, > > On 2018/3/2 9:59, Changwei Ge wrote: >> Hi Jun, >> I think the comments for both two functions are OK. >> No need to rework them. >> As we know, ocfs2 lock name(lock id) are composed of several parts including >> block number. > I looked though the comments involved 'lockid', and found 'lockid' is a > concept in dlm level, so ocfs2 level should not be aware of it. When reading source code of ocfs2 level, you'll find that 'lockid' is a new concept that has not occurred or be explained before. It's kind of difficult to understand. Maybe 'blockno' or 'blockid' is more sensible than 'lockid' I'm willing to fix them all. Thanks Larry > thanks, > Jun >> Thanks, >> Changw2ei >> >> On 2018/3/1 20:58, piaojun wrote: >>> Hi Larry, >>> >>> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >>> fixing them all? >>> >>> thanks, >>> Jun >>> >>> On 2018/2/28 18:17, Larry Chen wrote: >>>> The function ocfs2_double_lock tries to lock the inode with lower >>>> blockid first, not lockid. >>>> >>>> Signed-off-by: Larry Chen <lchen@suse.com> >>>> --- >>>> fs/ocfs2/namei.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>>> index c801eddc4bf3..30d454de35a8 100644 >>>> --- a/fs/ocfs2/namei.c >>>> +++ b/fs/ocfs2/namei.c >>>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>>> if (*bh2) >>>> *bh2 = NULL; >>>> >>>> - /* we always want to lock the one with the lower lockid first. >>>> + /* we always want to lock the one with the lower blockid first. >>>> * and if they are nested, we lock ancestor first */ >>>> if (oi1->ip_blkno != oi2->ip_blkno) { >>>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>>> >>> _______________________________________________ >>> Ocfs2-devel mailing list >>> Ocfs2-devel@oss.oracle.com >>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >>> >> . >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Ocfs2-devel] [PATCH] Correct a comment error 2018-03-02 2:15 ` piaojun @ 2018-03-02 2:38 ` Changwei Ge -1 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-02 2:38 UTC (permalink / raw) To: piaojun, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Jun, On 2018/3/2 10:16, piaojun wrote: > Hi Changwei, > > On 2018/3/2 9:59, Changwei Ge wrote: >> Hi Jun, >> I think the comments for both two functions are OK. >> No need to rework them. >> As we know, ocfs2 lock name(lock id) are composed of several parts including >> block number. > I looked though the comments involved 'lockid', and found 'lockid' is a > concept in dlm level, so ocfs2 level should not be aware of it. I don't agree. Please refer to ocfs2_build_lock_name(). DLM should not know how ocfs2 distinguishes objects it wants to protected. Moreover, ocfs2 has a abstraction layer called dlmglue. -Changwei > > thanks, > Jun >> >> Thanks, >> Changw2ei >> >> On 2018/3/1 20:58, piaojun wrote: >>> Hi Larry, >>> >>> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >>> fixing them all? >>> >>> thanks, >>> Jun >>> >>> On 2018/2/28 18:17, Larry Chen wrote: >>>> The function ocfs2_double_lock tries to lock the inode with lower >>>> blockid first, not lockid. >>>> >>>> Signed-off-by: Larry Chen <lchen@suse.com> >>>> --- >>>> fs/ocfs2/namei.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>>> index c801eddc4bf3..30d454de35a8 100644 >>>> --- a/fs/ocfs2/namei.c >>>> +++ b/fs/ocfs2/namei.c >>>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>>> if (*bh2) >>>> *bh2 = NULL; >>>> >>>> - /* we always want to lock the one with the lower lockid first. >>>> + /* we always want to lock the one with the lower blockid first. >>>> * and if they are nested, we lock ancestor first */ >>>> if (oi1->ip_blkno != oi2->ip_blkno) { >>>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>>> >>> >>> _______________________________________________ >>> Ocfs2-devel mailing list >>> Ocfs2-devel at oss.oracle.com >>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >>> >> . >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Ocfs2-devel] [PATCH] Correct a comment error @ 2018-03-02 2:38 ` Changwei Ge 0 siblings, 0 replies; 15+ messages in thread From: Changwei Ge @ 2018-03-02 2:38 UTC (permalink / raw) To: piaojun, Larry Chen, mfasheh@versity.com, jlbec@evilplan.org Cc: linux-kernel@vger.kernel.org, ocfs2-devel@oss.oracle.com Hi Jun, On 2018/3/2 10:16, piaojun wrote: > Hi Changwei, > > On 2018/3/2 9:59, Changwei Ge wrote: >> Hi Jun, >> I think the comments for both two functions are OK. >> No need to rework them. >> As we know, ocfs2 lock name(lock id) are composed of several parts including >> block number. > I looked though the comments involved 'lockid', and found 'lockid' is a > concept in dlm level, so ocfs2 level should not be aware of it. I don't agree. Please refer to ocfs2_build_lock_name(). DLM should not know how ocfs2 distinguishes objects it wants to protected. Moreover, ocfs2 has a abstraction layer called dlmglue. -Changwei > > thanks, > Jun >> >> Thanks, >> Changw2ei >> >> On 2018/3/1 20:58, piaojun wrote: >>> Hi Larry, >>> >>> There is the same mistake in ocfs2_reflink_inodes_lock(), could you help >>> fixing them all? >>> >>> thanks, >>> Jun >>> >>> On 2018/2/28 18:17, Larry Chen wrote: >>>> The function ocfs2_double_lock tries to lock the inode with lower >>>> blockid first, not lockid. >>>> >>>> Signed-off-by: Larry Chen <lchen@suse.com> >>>> --- >>>> fs/ocfs2/namei.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c >>>> index c801eddc4bf3..30d454de35a8 100644 >>>> --- a/fs/ocfs2/namei.c >>>> +++ b/fs/ocfs2/namei.c >>>> @@ -1133,7 +1133,7 @@ static int ocfs2_double_lock(struct ocfs2_super *osb, >>>> if (*bh2) >>>> *bh2 = NULL; >>>> >>>> - /* we always want to lock the one with the lower lockid first. >>>> + /* we always want to lock the one with the lower blockid first. >>>> * and if they are nested, we lock ancestor first */ >>>> if (oi1->ip_blkno != oi2->ip_blkno) { >>>> inode1_is_ancestor = ocfs2_check_if_ancestor(osb, oi2->ip_blkno, >>>> >>> >>> _______________________________________________ >>> Ocfs2-devel mailing list >>> Ocfs2-devel@oss.oracle.com >>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel >>> >> . >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-03-02 2:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-28 10:17 [Ocfs2-devel] [PATCH] Correct a comment error Larry Chen 2018-03-01 0:36 ` Changwei Ge 2018-03-01 0:36 ` Changwei Ge 2018-03-01 6:59 ` [Ocfs2-devel] 答复： " Lei Chen 2018-03-02 2:00 ` Changwei Ge 2018-03-01 12:56 ` [Ocfs2-devel] " piaojun 2018-03-01 12:56 ` piaojun 2018-03-02 1:59 ` Changwei Ge 2018-03-02 1:59 ` Changwei Ge 2018-03-02 2:15 ` piaojun 2018-03-02 2:15 ` piaojun 2018-03-02 2:37 ` Larry Chen 2018-03-02 2:37 ` Larry Chen 2018-03-02 2:38 ` Changwei Ge 2018-03-02 2:38 ` Changwei Ge
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.