chain_fsetxattr extra chunk removal

chain_fsetxattr extra chunk removal

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

Hi,

While writing unit tests for chain_xattr.cc I tried to understand how to create the conditions to trigger this part of the chain_fsetxattr function:

  /* if we're exactly at a chunk size, remove the next one (if wasn't removed
     before) */
  if (ret >= 0 && chunk_size == CHAIN_XATTR_MAX_BLOCK_LEN) {
    get_raw_xattr_name(name, i, raw_name, sizeof(raw_name));
    int r = sys_fremovexattr(fd, raw_name);
    if (r < 0 && r != -ENODATA)
      ret = r;
  }

I suspect this cleans up extra empty attributes created as a side effect of a previous version of the function. Or I just don't understand the case it addresses.

I'd very much appreciate a hint :-)

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: chain_fsetxattr extra chunk removal

[Yehuda Sadeh]

On Thu, Feb 7, 2013 at 12:59 PM, Loic Dachary <loic@dachary.org> wrote:
> Hi,
>
> While writing unit tests for chain_xattr.cc I tried to understand how to create the conditions to trigger this part of the chain_fsetxattr function:
>
>   /* if we're exactly at a chunk size, remove the next one (if wasn't removed
>      before) */
>   if (ret >= 0 && chunk_size == CHAIN_XATTR_MAX_BLOCK_LEN) {
>     get_raw_xattr_name(name, i, raw_name, sizeof(raw_name));
>     int r = sys_fremovexattr(fd, raw_name);
>     if (r < 0 && r != -ENODATA)
>       ret = r;
>   }
>
> I suspect this cleans up extra empty attributes created as a side effect of a previous version of the function. Or I just don't understand the case it addresses.
>
> I'd very much appreciate a hint :-)
>

Well, the code has changed a bit, but originally when a chain was
overwritten we didn't bother to remove the xattrs tail. When we read
the chain we stop either when we got a short xattr, or when the next
xattr in the chain didn't exist.  So when writing an xattr that was
perfectly aligned with the block len we had to remove the next xattr
in order make sure that readers will not over-read. I'm not too sure
whether that still the case, Sam might have a better idea.
In any case, it might be a good idea to test the case where we have a
big xattr that spans across multiple blocks (e.g., > 3) and being
overwritten by a short xattr. Probably also need to test it with
different combinations of aligned and non-aligned block sizes.

Thanks,
Yehuda

Re: chain_fsetxattr extra chunk removal

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]



On 02/11/2013 06:13 AM, Yehuda Sadeh wrote:
> On Thu, Feb 7, 2013 at 12:59 PM, Loic Dachary <loic@dachary.org> wrote:
>> Hi,
>>
>> While writing unit tests for chain_xattr.cc I tried to understand how to create the conditions to trigger this part of the chain_fsetxattr function:
>>
>>   /* if we're exactly at a chunk size, remove the next one (if wasn't removed
>>      before) */
>>   if (ret >= 0 && chunk_size == CHAIN_XATTR_MAX_BLOCK_LEN) {
>>     get_raw_xattr_name(name, i, raw_name, sizeof(raw_name));
>>     int r = sys_fremovexattr(fd, raw_name);
>>     if (r < 0 && r != -ENODATA)
>>       ret = r;
>>   }
>>
>> I suspect this cleans up extra empty attributes created as a side effect of a previous version of the function. Or I just don't understand the case it addresses.
>>
>> I'd very much appreciate a hint :-)
>>
> 
> Well, the code has changed a bit, but originally when a chain was
> overwritten we didn't bother to remove the xattrs tail. When we read
> the chain we stop either when we got a short xattr, or when the next
> xattr in the chain didn't exist.  So when writing an xattr that was
> perfectly aligned with the block len we had to remove the next xattr
> in order make sure that readers will not over-read. I'm not too sure
> whether that still the case, Sam might have a better idea.
> In any case, it might be a good idea to test the case where we have a
> big xattr that spans across multiple blocks (e.g., > 3) and being
> overwritten by a short xattr. Probably also need to test it with
> different combinations of aligned and non-aligned block sizes.

I understand now and I'll modify the pull request https://github.com/ceph/ceph/pull/40 accordingly.

Thanks :-)

> 
> Thanks,
> Yehuda
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: chain_fsetxattr extra chunk removal

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

Hi,

I amended the unit tests ( https://github.com/ceph/ceph/pull/40/files ) to cover the code below. A review would be much appreciated :-)

Cheers

On 02/11/2013 09:08 PM, Loic Dachary wrote:
> 
> 
> On 02/11/2013 06:13 AM, Yehuda Sadeh wrote:
>> On Thu, Feb 7, 2013 at 12:59 PM, Loic Dachary <loic@dachary.org> wrote:
>>> Hi,
>>>
>>> While writing unit tests for chain_xattr.cc I tried to understand how to create the conditions to trigger this part of the chain_fsetxattr function:
>>>
>>>   /* if we're exactly at a chunk size, remove the next one (if wasn't removed
>>>      before) */
>>>   if (ret >= 0 && chunk_size == CHAIN_XATTR_MAX_BLOCK_LEN) {
>>>     get_raw_xattr_name(name, i, raw_name, sizeof(raw_name));
>>>     int r = sys_fremovexattr(fd, raw_name);
>>>     if (r < 0 && r != -ENODATA)
>>>       ret = r;
>>>   }
>>>
>>> I suspect this cleans up extra empty attributes created as a side effect of a previous version of the function. Or I just don't understand the case it addresses.
>>>
>>> I'd very much appreciate a hint :-)
>>>
>>
>> Well, the code has changed a bit, but originally when a chain was
>> overwritten we didn't bother to remove the xattrs tail. When we read
>> the chain we stop either when we got a short xattr, or when the next
>> xattr in the chain didn't exist.  So when writing an xattr that was
>> perfectly aligned with the block len we had to remove the next xattr
>> in order make sure that readers will not over-read. I'm not too sure
>> whether that still the case, Sam might have a better idea.
>> In any case, it might be a good idea to test the case where we have a
>> big xattr that spans across multiple blocks (e.g., > 3) and being
>> overwritten by a short xattr. Probably also need to test it with
>> different combinations of aligned and non-aligned block sizes.
> 
> I understand now and I'll modify the pull request https://github.com/ceph/ceph/pull/40 accordingly.
> 
> Thanks :-)
> 
>>
>> Thanks,
>> Yehuda
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]