diff mbox

ocfs2: drop the unwanted null byte in dentry lock name

Message ID 5295B5BC.5020905@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joseph Qi Nov. 27, 2013, 9:05 a.m. UTC 
Dentry lock name is made up of the type 'N', the parent blkno and its
blkno represented as a binary value.
Now there is a null byte between parent blkno and the binary blkno
value, so drop it.

Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/dlm/dlmdebug.c | 2 +-
 fs/ocfs2/dlmglue.c      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Andrew Morton Nov. 27, 2013, 11:38 p.m. UTC | #1 
On Wed, 27 Nov 2013 17:05:00 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:

> Dentry lock name is made up of the type 'N', the parent blkno and its
> blkno represented as a binary value.
> Now there is a null byte between parent blkno and the binary blkno
> value, so drop it.

Geeze.

What are the runtime effects of this change?

> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -257,7 +257,7 @@ static int stringify_lockname(const char *lockname, int locklen, char *buf,
>  #define OCFS2_DENTRY_LOCK_INO_START	18
>  	if (*lockname == 'N') {
>  		memcpy((__be64 *)&inode_blkno_be,
> -		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START],
> +		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START - 1],

And what's up with all the CrazyCasting?  &inode_blkno_be is *already*
__be64* and the second arg to memcpy is `const void *', so the cast
there is also unneeded.

>  		       sizeof(__be64));
>  		out += snprintf(buf + out, len - out, "%.*s%08x",
>  				OCFS2_DENTRY_LOCK_INO_START - 1, lockname,
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index 3a44a64..62f2387 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -586,7 +586,7 @@ static __u64 ocfs2_get_dentry_lock_ino(struct ocfs2_lock_res *lockres)
>  {
>  	__be64 inode_blkno_be;
>  
> -	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START],
> +	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1],

Like that.

>  	       sizeof(__be64));
>  
>  	return be64_to_cpu(inode_blkno_be);
> @@ -631,7 +631,7 @@ void ocfs2_dentry_lock_res_init(struct ocfs2_dentry_lock *dl,
>  
>  	BUG_ON(len != (OCFS2_DENTRY_LOCK_INO_START - 1));
>  
> -	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START], &inode_blkno_be,
> +	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1], &inode_blkno_be,
>  	       sizeof(__be64));
>  
>  	ocfs2_lock_res_init_common(OCFS2_SB(inode->i_sb), lockres,
> -- 
> 1.8.4.3
Joseph Qi Nov. 28, 2013, 1:03 a.m. UTC | #2 
On 2013/11/28 7:38, Andrew Morton wrote:
> On Wed, 27 Nov 2013 17:05:00 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> 
>> Dentry lock name is made up of the type 'N', the parent blkno and its
>> blkno represented as a binary value.
>> Now there is a null byte between parent blkno and the binary blkno
>> value, so drop it.
> 
> Geeze.
> 
> What are the runtime effects of this change?
> 

If the null byte exists, it is obviously we cannot print the full dentry
lock name using '%s'.
But the fact is, the blkno is stuffed with binary value, it may also
contain null bytes, so we still cannot get the full dentry lock name in
the message.
Maybe we should also distinguish dentry lock and other lockres when
print lock name. But this makes the code malformed.

>> --- a/fs/ocfs2/dlm/dlmdebug.c
>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>> @@ -257,7 +257,7 @@ static int stringify_lockname(const char *lockname, int locklen, char *buf,
>>  #define OCFS2_DENTRY_LOCK_INO_START	18
>>  	if (*lockname == 'N') {
>>  		memcpy((__be64 *)&inode_blkno_be,
>> -		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START],
>> +		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START - 1],
> 
> And what's up with all the CrazyCasting?  &inode_blkno_be is *already*
> __be64* and the second arg to memcpy is `const void *', so the cast
> there is also unneeded.
> 
>>  		       sizeof(__be64));
>>  		out += snprintf(buf + out, len - out, "%.*s%08x",
>>  				OCFS2_DENTRY_LOCK_INO_START - 1, lockname,
>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>> index 3a44a64..62f2387 100644
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -586,7 +586,7 @@ static __u64 ocfs2_get_dentry_lock_ino(struct ocfs2_lock_res *lockres)
>>  {
>>  	__be64 inode_blkno_be;
>>  
>> -	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START],
>> +	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1],
> 
> Like that.
> 
>>  	       sizeof(__be64));
>>  
>>  	return be64_to_cpu(inode_blkno_be);
>> @@ -631,7 +631,7 @@ void ocfs2_dentry_lock_res_init(struct ocfs2_dentry_lock *dl,
>>  
>>  	BUG_ON(len != (OCFS2_DENTRY_LOCK_INO_START - 1));
>>  
>> -	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START], &inode_blkno_be,
>> +	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1], &inode_blkno_be,
>>  	       sizeof(__be64));
>>  
>>  	ocfs2_lock_res_init_common(OCFS2_SB(inode->i_sb), lockres,
>> -- 
>> 1.8.4.3
> 
> .
>
Mark Fasheh Nov. 28, 2013, 4:40 p.m. UTC | #3 
On Wed, Nov 27, 2013 at 03:38:41PM -0800, Andrew Morton wrote:
> On Wed, 27 Nov 2013 17:05:00 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> 
> > Dentry lock name is made up of the type 'N', the parent blkno and its
> > blkno represented as a binary value.
> > Now there is a null byte between parent blkno and the binary blkno
> > value, so drop it.
> 
> Geeze.
> 
> What are the runtime effects of this change?

This will change the over-the-wire protocol, so big NACK for this
patch please. Some of the lock names are binary and have been forever now.
We have a limited amount of space in the name and some lock types would have
names too long if we stringified every field.
	--Mark

--
Mark Fasheh
Joseph Qi Nov. 30, 2013, 1:45 a.m. UTC | #4 
On 2013/11/29 0:40, Mark Fasheh wrote:
> On Wed, Nov 27, 2013 at 03:38:41PM -0800, Andrew Morton wrote:
>> On Wed, 27 Nov 2013 17:05:00 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
>>
>>> Dentry lock name is made up of the type 'N', the parent blkno and its
>>> blkno represented as a binary value.
>>> Now there is a null byte between parent blkno and the binary blkno
>>> value, so drop it.
>>
>> Geeze.
>>
>> What are the runtime effects of this change?
> 
> This will change the over-the-wire protocol, so big NACK for this
> patch please. Some of the lock names are binary and have been forever now.
> We have a limited amount of space in the name and some lock types would have
> names too long if we stringified every field.
> 	--Mark
> 
What do you mean by change the over-the-wire protocol?
In my opinion, the only thing we should do is to make buildup and parse
consistent.
But if the new version is mixed mounted with the old, it may have
problem because of the changed protocol.

> --
> Mark Fasheh
> 
>
Mark Fasheh Dec. 1, 2013, 11:16 p.m. UTC | #5 
On Sat, Nov 30, 2013 at 09:45:04AM +0800, Joseph Qi wrote:
> On 2013/11/29 0:40, Mark Fasheh wrote:
> > On Wed, Nov 27, 2013 at 03:38:41PM -0800, Andrew Morton wrote:
> >> On Wed, 27 Nov 2013 17:05:00 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> >>
> >>> Dentry lock name is made up of the type 'N', the parent blkno and its
> >>> blkno represented as a binary value.
> >>> Now there is a null byte between parent blkno and the binary blkno
> >>> value, so drop it.
> >>
> >> Geeze.
> >>
> >> What are the runtime effects of this change?
> > 
> > This will change the over-the-wire protocol, so big NACK for this
> > patch please. Some of the lock names are binary and have been forever now.
> > We have a limited amount of space in the name and some lock types would have
> > names too long if we stringified every field.
> > 	--Mark
> > 
> What do you mean by change the over-the-wire protocol?
> In my opinion, the only thing we should do is to make buildup and parse
> consistent.
> But if the new version is mixed mounted with the old, it may have
> problem because of the changed protocol.

That's exactly what "change the over-the-wire protocol" means - "new" and
old versions of the file will not be able to communicate. This is
unacceptable and will anger our users.

We don't make changes like that just to clean up fields.
	--Mark

--
Mark Fasheh
diff mbox

Patch

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index e33cd7a..2d85dfa 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -257,7 +257,7 @@  static int stringify_lockname(const char *lockname, int locklen, char *buf,
 #define OCFS2_DENTRY_LOCK_INO_START	18
 	if (*lockname == 'N') {
 		memcpy((__be64 *)&inode_blkno_be,
-		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START],
+		       (char *)&lockname[OCFS2_DENTRY_LOCK_INO_START - 1],
 		       sizeof(__be64));
 		out += snprintf(buf + out, len - out, "%.*s%08x",
 				OCFS2_DENTRY_LOCK_INO_START - 1, lockname,
diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index 3a44a64..62f2387 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -586,7 +586,7 @@  static __u64 ocfs2_get_dentry_lock_ino(struct ocfs2_lock_res *lockres)
 {
 	__be64 inode_blkno_be;
 
-	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START],
+	memcpy(&inode_blkno_be, &lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1],
 	       sizeof(__be64));
 
 	return be64_to_cpu(inode_blkno_be);
@@ -631,7 +631,7 @@  void ocfs2_dentry_lock_res_init(struct ocfs2_dentry_lock *dl,
 
 	BUG_ON(len != (OCFS2_DENTRY_LOCK_INO_START - 1));
 
-	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START], &inode_blkno_be,
+	memcpy(&lockres->l_name[OCFS2_DENTRY_LOCK_INO_START - 1], &inode_blkno_be,
 	       sizeof(__be64));
 
 	ocfs2_lock_res_init_common(OCFS2_SB(inode->i_sb), lockres,