[sheepdog] [PATCH] md: don't use xattr for calculating path space

[Kai Zhang]

On May 23, 2013, at 5:01 PM, Liu Yuan <namei.unix at gmail.com> wrote:

> On 05/23/2013 04:13 PM, Kai Zhang wrote:
>> Currently, md calls init_path_space() for calculating path space after a path is pluged.
>> If the path is pluged for the first time, md will use statvfs() to calculate path space
>> ant set it to xattr. Next time when md calls init_path_space(), it will try to get path
>> space from xattr other than re-calculate it.
>> 
>> This works fine for most times.
>> However, if the path space changes for some reasons (for example, a lvm resize operation),
>> init_path_space() will return a wrong number, because xattr haven't changed.
>> 
>> This patch will let init_path_space() use statvfs() for calculating space for each time.
>> 
>> Signed-off-by: Kai Zhang <kyle at zelin.io>
>> ---
>> sheep/md.c |   31 ++++---------------------------
>> 1 files changed, 4 insertions(+), 27 deletions(-)
>> 
>> diff --git a/sheep/md.c b/sheep/md.c
>> index 291ffca..60dbd04 100644
>> --- a/sheep/md.c
>> +++ b/sheep/md.c
>> @@ -255,47 +255,24 @@ out:
>> }
>> 
>> /*
>> - * If path is broken during initilization or not support xattr return 0. We can
>> + * If path is broken during initilization return 0. We can
>>  * safely use 0 to represent failure case  because 0 space path can be
>>  * considered as broken path.
>>  */
>> static uint64_t init_path_space(char *path)
>> {
>> -	uint64_t size;
>> +	uint64_t size = 0;
>> 	char stale[PATH_MAX];
>> 
>> -	if (!is_xattr_enabled(path)) {
>> -		sd_iprintf("multi-disk support need xattr feature");
>> -		goto broken_path;
>> -	}
>> -
>> 	snprintf(stale, PATH_MAX, "%s/.stale", path);
>> 	if (xmkdir(stale, sd_def_dmode) < 0) {
>> 		sd_eprintf("can't mkdir for %s, %m", stale);
>> -		goto broken_path;
>> -	}
>> -
>> -	if (getxattr(path, MDNAME, &size, MDSIZE) < 0) {
>> -		if (errno == ENODATA) {
>> -			goto create;
>> -		} else {
>> -			sd_eprintf("%s, %m", path);
>> -			goto broken_path;
>> -		}
>> +		goto out;
>> 	}
>> 
>> -	return size;
>> -create:
>> 	size = get_path_size(path, NULL);
>> -	if (!size)
>> -		goto broken_path;
>> -	if (setxattr(path, MDNAME, &size, MDSIZE, 0) < 0) {
>> -		sd_eprintf("%s, %m", path);
>> -		goto broken_path;
>> -	}
>> +out:
>> 	return size;
>> -broken_path:
>> -	return 0;
>> }
>> 
>> static inline void remove_disk(int idx)
>> 
> 
> This looks not enough to me. We use get_path_size() to get the actually
> total capacity, so you should fix get_path_size() too, no? After node
> reboot, we just get what is free size.
> 
> 

Thanks for catching this.

I think the name of get_path_size() makes me confused. 
Actually, it returns free size and also set the second parameter as used size.

How about modify it to:  

int get_path_size(char *path, uint64_t *used, uint64_t *free, uint64_t *total)

Thanks,
Kyle