Re: [PATCH v2 20/35] ubifs: implement IO functions for quota files

[Dongsheng Yang <yangds.fnst@cn.fujitsu.com>]

On 08/04/2015 05:46 AM, Richard Weinberger wrote:
> Am 30.07.2015 um 07:48 schrieb Dongsheng Yang:
>> We have to implement 3 functions to support quota in ubifs:
>> ubifs_quota_read(), ubifs_quota_write() and ubifs_get_quotas()
>>
>> ubifs_quota_read():
>> 	callback to read quota file.
>> ubifs_quota_write():
>> 	callback to write data to quota file.
>> ubifs_get_quotas():
>> 	callback to get quota instance for inode.
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   fs/ubifs/super.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 219 insertions(+)
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index bc57685..3609d7b 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -940,6 +940,220 @@ static int check_volume_empty(struct ubifs_info *c)
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_QUOTA
>> +static ssize_t ubifs_quota_read(struct super_block *sb, int type, char *data,
>> +			       size_t len, loff_t off)
>> +{
>> +	struct inode *inode = sb_dqopt(sb)->files[type];
>> +	unsigned long block = off >> UBIFS_BLOCK_SHIFT;
>> +	int offset = off & (sb->s_blocksize - 1);
>> +	int tocopy = 0;
>> +	size_t toread;
>> +	char *block_buf;
>> +	struct ubifs_data_node *dn;
>> +	int ret, err = 0;
>> +
>> +	toread = len;
>
> Minor nit, please group same types together and also group assignments
> as other UBI/FS codes does.

Hm, okey. Good suggestion.
>
>> +	dn = kmalloc(UBIFS_MAX_DATA_NODE_SZ, GFP_NOFS);
>> +	if (!dn) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	block_buf = kmalloc(sb->s_blocksize, GFP_NOFS);
>> +	if (!block_buf) {
>> +		err = -ENOMEM;
>> +		goto free_dn;
>> +	}
>> +
>> +	if (offset) {
>> +		/* Read the un-aligned data in first block */
>> +		tocopy = sb->s_blocksize - offset;
>> +		if (toread < tocopy)
>> +			tocopy = toread;
>
> min()?

okey, thanx
>
>> +		ret = ubifs_read_block(inode, block_buf, block, dn);
>> +		if (ret) {
>> +			if (ret != -ENOENT) {
>
> if (ret && ret != -ENOENT)?

Yea, agree.
>
>> +				err = ret;
>> +				goto free_buf;
>> +			}
>> +		}
>> +
>> +		memcpy(data, block_buf + offset, tocopy);
>> +
>> +		block++;
>> +		toread -= tocopy;
>> +		data += tocopy;
>> +		tocopy = 0;
>> +	}
>> +
>> +	while (toread > 0) {
>> +		tocopy = sb->s_blocksize < toread ?
>> +				sb->s_blocksize : toread;
>
> min()?

Yes, correct.
>
>> +
>> +		/* Break to read the last block */
>> +		if (tocopy < sb->s_blocksize)
>> +			break;
>
> Hmm, can't you combine this check with the above condition?

Sure.
>
>> +		ret = ubifs_read_block(inode, data, block, dn);
>> +		if (ret) {
>> +			if (ret != -ENOENT) {
>
> See above.

Thanx
>
>> +				err = ret;
>> +				goto free_buf;
>> +			}
>> +		}
>> +		block++;;
>
> superfluous semicolon.

Oh, right you are.
>
>> +		toread -= tocopy;
>> +		data += tocopy;
>> +		tocopy = 0;
>> +	}
>> +
>> +	if (tocopy) {
>> +		/* Read the data in last block */
>> +		ret = ubifs_read_block(inode, block_buf, block, dn);
>> +		if (ret) {
>> +			if (ret != -ENOENT) {
>
> See above. Maybe the -ENOENT stuff can be integrated into ubifs_read_block().
> All callers deal with it the same way...

Good point. Will try it.
>
>> +				err = ret;
>> +				goto free_buf;
>> +			}
>> +		}
>> +
>> +		memcpy(data, block_buf, tocopy);
>> +	}
>> +free_buf:
>> +	kfree(block_buf);
>> +free_dn:
>> +	kfree(dn);
>> +out:
>> +	if (!err)
>> +		return len;
>> +	return err;
>> +}
>> +
>> +static ssize_t ubifs_quota_write(struct super_block *sb, int type,
>> +				const char *data, size_t len, loff_t off)
>> +{
>> +	struct inode *inode = sb_dqopt(sb)->files[type];
>> +	unsigned long block = off >> UBIFS_BLOCK_SHIFT;
>> +	struct ubifs_info *c = inode->i_sb->s_fs_info;
>> +	int offset = off & (sb->s_blocksize - 1);
>> +	union ubifs_key key;
>> +	int tocopy = 0;
>> +	size_t towrite = len;
>> +	int ret, err = 0;
>> +	struct ubifs_budget_req req = {};
>> +	int new_block = 0;
>> +	struct ubifs_data_node *dn;
>> +	char *block_buf;
>
> See above.
>
>> +	/* There are dirtied blocks and new blockes, but we call them all
>> +	 * new blocks here, the budgeting is same for them.
>> +	 */
>> +	new_block = DIV_ROUND_UP(len - (sb->s_blocksize - offset), sb->s_blocksize);
>> +	if (offset)
>> +		new_block += 1;
>> +
>> +	req.new_block_num = new_block;
>> +
>> +	err = ubifs_budget_space(c, &req);
>> +	if (err)
>> +		goto out;
>> +
>> +	dn = kmalloc(UBIFS_MAX_DATA_NODE_SZ, GFP_NOFS);
>> +	if (!dn) {
>> +		err = -ENOMEM;
>> +		goto release_budget;
>> +	}
>> +
>> +	block_buf = kmalloc(sb->s_blocksize, GFP_NOFS);
>> +	if (!block_buf) {
>> +		err = -ENOMEM;
>> +		goto free_dn;
>> +	}
>> +
>> +	if (offset) {
>> +		/* Write the un-aligned data in first block */
>> +		tocopy = sb->s_blocksize - offset;
>> +		if (towrite < tocopy)
>> +			tocopy = towrite;
>
> See above.
>
>> +		ret = ubifs_read_block(inode, block_buf, block, dn);
>> +		if (ret) {
>> +			if (ret != -ENOENT) {
>> +				err = ret;
>> +				goto free_buf;
>> +			}
>> +			memset(block_buf, 0, sb->s_blocksize);
>> +		}
>> +
>> +		memcpy(block_buf + offset, data, tocopy);
>> +
>> +		data_key_init(c, &key, inode->i_ino, block);
>> +		err = ubifs_jnl_write_data(c, inode, &key, block_buf, sb->s_blocksize);
>
> Hmm, I'm confused. You write the block containing the quota information to an inode's data?
> How can be determined whether data is real data and what is quota data?

Ha, so we need a special writing function for quota file, 
xxxfs_quota_write().
This function would bypass the quota recording, so quota
data would not be contained in by itself.
> This clearly shows that I know not much about the quota subsystem. 8^)
>
>> +		if (err) {
>> +			goto free_buf;
>> +		}
>> +		block++;
>> +		towrite -= tocopy;
>> +		data += tocopy;
>> +		tocopy = 0;
>> +	}
>> +
>> +	while (towrite > 0) {
>> +		tocopy = sb->s_blocksize < towrite ?
>> +				sb->s_blocksize : towrite;
>> +
>> +		/* Break to read the last block */
>> +		if (tocopy < sb->s_blocksize)
>> +			break;
>
> See above. Maybe you can combine all this common code.

Agree, thanx
>
>> +		data_key_init(c, &key, inode->i_ino, block);
>> +		err = ubifs_jnl_write_data(c, inode, &key, data, sb->s_blocksize);
>> +		if (err)
>> +			goto free_buf;
>> +		block++;;
>
> See above.
>
> I'll review your other patches these days.
> For now I'm too sleepy.

Thanx a lot Richard.

Your review is always helpful to me.

Yang
>
> Thanks,
> //richard
> .
>