Re: [PATCH 3/5] ceph: make use of ceph file layout helpers

[Alex Elder <elder@dreamhost.com>]

On 03/16/2012 12:59 AM, Sage Weil wrote:
> On Mon, 12 Mar 2012, Alex Elder wrote:
>> There are helpers defined in "include/linux/ceph/osdmap.h" for
>> accessing the fields of an on-disk ceph file layout structure.
>> Use them--consistently--throughout the code.
>>
>> Also define CEPH_FILE_LAYOUT_PG_PREFERRED_NONE, to represent
>> symbolically the explicit "no preferred PG" value.
>>
>> Make a few casts explicit too (to make it more obvious it's
>> occuring).  This produces some long lines, but they go away in an
>> upcoming patch.
>>
>> Signed-off-by: Alex Elder<elder@dreamhost.com>

A few comments below.  I'm about to re-post this series.

. . .

>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>> index 75960b1..bfd5b93 100644
>> --- a/fs/ceph/xattr.c
>> +++ b/fs/ceph/xattr.c
>> @@ -115,11 +115,12 @@ static size_t ceph_vxattrcb_file_layout(struct
>> ceph_inode_info *ci, char *val,
>>
>>   	ret = snprintf(val, size,
>>   		"chunk_bytes=%lld\nstripe_count=%lld\nobject_size=%lld\n",
>> -		(unsigned long long)ceph_file_layout_su(&ci->i_layout),
>> +		(unsigned long
>> long)ceph_file_layout_stripe_unit(&ci->i_layout),
>>   		(unsigned long
>> long)ceph_file_layout_stripe_count(&ci->i_layout),
>>   		(unsigned long
>> long)ceph_file_layout_object_size(&ci->i_layout));
>>
>> -	if (ceph_file_layout_pg_preferred(&ci->i_layout)>= 0) {
>> +	if (ceph_file_layout_pg_preferred(&ci->i_layout) !=
>> +			CEPH_FILE_LAYOUT_PG_PREFERRED_NONE) {
>
> The function returns cpu order, #define is little endian

I will fix this--throughout.  I'm going to define the constant
in CPU order instead (and fix the one place that depended on it
being in little endian order).

>>   		val += ret;
>>   		size -= ret;
>>   		ret += snprintf(val, size, "preferred_osd=%lld\n",
>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
>> index 2645704..cf86032 100644
>> --- a/include/linux/ceph/ceph_fs.h
>> +++ b/include/linux/ceph/ceph_fs.h

. . .

>> @@ -1011,23 +1011,24 @@ int ceph_calc_object_layout(struct ceph_object_layout
>> *ol,
>>   	if (!pool)
>>   		return -EIO;
>>   	ps = ceph_str_hash(pool->v.object_hash, oid, strlen(oid));
>> -	if (preferred>= 0) {
>> +
>> +	if (preferred == CEPH_FILE_LAYOUT_PG_PREFERRED_NONE) {
>
> Here, too.  I'd just stick with>= ...

No, I intentionally defined this constant to have a specific
meaning, and I don't want any sense that it is greater than
or less than something--just distinguished from all other
valid preferred PG values.

>
>> +		num = le32_to_cpu(pool->v.pg_num);
>> +		num_mask = pool->pg_num_mask;
>> +	} else {
>>   		ps += preferred;
>>   		num = le32_to_cpu(pool->v.lpg_num);
>>   		num_mask = pool->lpg_num_mask;
>> -	} else {

. . .