diff mbox

[v3,36/39] ubifs: implement ubifs_get_qsize to get quota size in ubifs

Message ID 1442307754-13233-37-git-send-email-yangds.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Yang Dongsheng Sept. 15, 2015, 9:02 a.m. UTC
We only care the size of regular file in ubifs for quota.
The reason is similar with the comment in ubifs_getattr().

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 fs/ubifs/dir.c   |  3 +++
 fs/ubifs/file.c  | 22 ++++++++++++++++++++++
 fs/ubifs/ubifs.h |  1 +
 3 files changed, 26 insertions(+)

Comments

Jan Kara Sept. 16, 2015, 10 a.m. UTC | #1
On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
> We only care the size of regular file in ubifs for quota.
> The reason is similar with the comment in ubifs_getattr().
> 
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>  fs/ubifs/dir.c   |  3 +++
>  fs/ubifs/file.c  | 22 ++++++++++++++++++++++
>  fs/ubifs/ubifs.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index 802c6ad..0d3d6d3 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>  	.update_time = ubifs_update_time,
>  #endif
> +#ifdef CONFIG_QUOTA
> +	.get_qsize	= ubifs_get_qsize,
> +#endif
>  };
>  
>  const struct file_operations ubifs_dir_operations = {
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index b57ccf3..f1d792a 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
>  	return 0;
>  }
>  
> +/*
> + * ubifs_get_qsize: get the quota size of a file
> + * @inode: inode which we are going to get the qsize
> + *
> + * We only care the size of regular file in ubifs
> + * for quota. The reason is similar with the comment
> + * in ubifs_getattr().
> + */
> +ssize_t ubifs_get_qsize(struct inode *inode)
> +{
> +	if (S_ISREG(inode->i_mode))
> +		return i_size_read(inode);
> +	else
> +		return 0;
> +}
> +

The quota space is accounted in bytes. So why don't you store appropriate
number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
have files occupying only say 100 bytes and everything works properly
there so I don't see why ubifs needs to differ.

								Honza

>  const struct address_space_operations ubifs_file_address_operations = {
>  	.readpage       = ubifs_readpage,
>  	.writepage      = ubifs_writepage,
> @@ -1656,6 +1672,9 @@ const struct inode_operations ubifs_file_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>  	.update_time = ubifs_update_time,
>  #endif
> +#ifdef CONFIG_QUOTA
> +	.get_qsize	= ubifs_get_qsize,
> +#endif
>  };
>  
>  const struct inode_operations ubifs_symlink_inode_operations = {
> @@ -1670,6 +1689,9 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>  	.update_time = ubifs_update_time,
>  #endif
> +#ifdef CONFIG_QUOTA
> +	.get_qsize	= ubifs_get_qsize,
> +#endif
>  };
>  
>  const struct file_operations ubifs_file_operations = {
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 99cf10c..21b5dc0 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1759,6 +1759,7 @@ int ubifs_read_block(struct inode *inode, void *addr, unsigned int block,
>  int ubifs_fsync(struct file *file, loff_t start, loff_t end, int datasync);
>  int ubifs_setattr(struct dentry *dentry, struct iattr *attr);
>  int ubifs_update_time(struct inode *inode, struct timespec *time, int flags);
> +ssize_t ubifs_get_qsize(struct inode *inode);
>  
>  /* dir.c */
>  struct inode *ubifs_new_inode(struct ubifs_info *c, const struct inode *dir,
> -- 
> 1.8.4.2
>
Yang Dongsheng Sept. 17, 2015, 7:23 a.m. UTC | #2
On 09/16/2015 06:00 PM, Jan Kara wrote:
> On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
>> We only care the size of regular file in ubifs for quota.
>> The reason is similar with the comment in ubifs_getattr().
>>
>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>> ---
>>   fs/ubifs/dir.c   |  3 +++
>>   fs/ubifs/file.c  | 22 ++++++++++++++++++++++
>>   fs/ubifs/ubifs.h |  1 +
>>   3 files changed, 26 insertions(+)
>>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 802c6ad..0d3d6d3 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
>>   #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>   	.update_time = ubifs_update_time,
>>   #endif
>> +#ifdef CONFIG_QUOTA
>> +	.get_qsize	= ubifs_get_qsize,
>> +#endif
>>   };
>>
>>   const struct file_operations ubifs_dir_operations = {
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index b57ccf3..f1d792a 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
>>   	return 0;
>>   }
>>
>> +/*
>> + * ubifs_get_qsize: get the quota size of a file
>> + * @inode: inode which we are going to get the qsize
>> + *
>> + * We only care the size of regular file in ubifs
>> + * for quota. The reason is similar with the comment
>> + * in ubifs_getattr().
>> + */
>> +ssize_t ubifs_get_qsize(struct inode *inode)
>> +{
>> +	if (S_ISREG(inode->i_mode))
>> +		return i_size_read(inode);
>> +	else
>> +		return 0;
>> +}
>> +
>
> The quota space is accounted in bytes. So why don't you store appropriate
> number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
> have files occupying only say 100 bytes and everything works properly
> there so I don't see why ubifs needs to differ.

Ha, yes, we did not keep i_blocks in ubifs currently. Because we have
no blocks in ubifs. Although we can simulate a i_block for quota, I
did not do it. Let me try to show what I am thinking here.

(1).Block file system are counting space with blocks. Then the quota
could works in (dquot_alloc_block & i_blocks) way. I mean, account
spaces by dquot_alloc_block() and FIOQSIZE can get the qsize from
i_blocks.

(2). But ubifs has no blocks, then I choose another way to do it,
(quot_alloc_space & i_size). That means, we account quota spaces
in ubifs by dquot_alloc_space() and want FIOSIZE to get i_size
of inodes. Then there is no notion of *block* but only space.

So, I want to make FIOSIZE more flexible here to introduce a get_qsize()
into inode_operations.

Yang
>
> 								Honza
>
>>   const struct address_space_operations ubifs_file_address_operations = {
>>   	.readpage       = ubifs_readpage,
>>   	.writepage      = ubifs_writepage,
>> @@ -1656,6 +1672,9 @@ const struct inode_operations ubifs_file_inode_operations = {
>>   #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>   	.update_time = ubifs_update_time,
>>   #endif
>> +#ifdef CONFIG_QUOTA
>> +	.get_qsize	= ubifs_get_qsize,
>> +#endif
>>   };
>>
>>   const struct inode_operations ubifs_symlink_inode_operations = {
>> @@ -1670,6 +1689,9 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>>   #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>   	.update_time = ubifs_update_time,
>>   #endif
>> +#ifdef CONFIG_QUOTA
>> +	.get_qsize	= ubifs_get_qsize,
>> +#endif
>>   };
>>
>>   const struct file_operations ubifs_file_operations = {
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index 99cf10c..21b5dc0 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -1759,6 +1759,7 @@ int ubifs_read_block(struct inode *inode, void *addr, unsigned int block,
>>   int ubifs_fsync(struct file *file, loff_t start, loff_t end, int datasync);
>>   int ubifs_setattr(struct dentry *dentry, struct iattr *attr);
>>   int ubifs_update_time(struct inode *inode, struct timespec *time, int flags);
>> +ssize_t ubifs_get_qsize(struct inode *inode);
>>
>>   /* dir.c */
>>   struct inode *ubifs_new_inode(struct ubifs_info *c, const struct inode *dir,
>> --
>> 1.8.4.2
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Sept. 17, 2015, noon UTC | #3
On Thu 17-09-15 15:23:11, Dongsheng Yang wrote:
> On 09/16/2015 06:00 PM, Jan Kara wrote:
> >On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
> >>We only care the size of regular file in ubifs for quota.
> >>The reason is similar with the comment in ubifs_getattr().
> >>
> >>Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> >>---
> >>  fs/ubifs/dir.c   |  3 +++
> >>  fs/ubifs/file.c  | 22 ++++++++++++++++++++++
> >>  fs/ubifs/ubifs.h |  1 +
> >>  3 files changed, 26 insertions(+)
> >>
> >>diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> >>index 802c6ad..0d3d6d3 100644
> >>--- a/fs/ubifs/dir.c
> >>+++ b/fs/ubifs/dir.c
> >>@@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
> >>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
> >>  	.update_time = ubifs_update_time,
> >>  #endif
> >>+#ifdef CONFIG_QUOTA
> >>+	.get_qsize	= ubifs_get_qsize,
> >>+#endif
> >>  };
> >>
> >>  const struct file_operations ubifs_dir_operations = {
> >>diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> >>index b57ccf3..f1d792a 100644
> >>--- a/fs/ubifs/file.c
> >>+++ b/fs/ubifs/file.c
> >>@@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
> >>  	return 0;
> >>  }
> >>
> >>+/*
> >>+ * ubifs_get_qsize: get the quota size of a file
> >>+ * @inode: inode which we are going to get the qsize
> >>+ *
> >>+ * We only care the size of regular file in ubifs
> >>+ * for quota. The reason is similar with the comment
> >>+ * in ubifs_getattr().
> >>+ */
> >>+ssize_t ubifs_get_qsize(struct inode *inode)
> >>+{
> >>+	if (S_ISREG(inode->i_mode))
> >>+		return i_size_read(inode);
> >>+	else
> >>+		return 0;
> >>+}
> >>+
> >
> >The quota space is accounted in bytes. So why don't you store appropriate
> >number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
> >have files occupying only say 100 bytes and everything works properly
> >there so I don't see why ubifs needs to differ.
> 
> Ha, yes, we did not keep i_blocks in ubifs currently. Because we have
> no blocks in ubifs. Although we can simulate a i_block for quota, I
> did not do it. Let me try to show what I am thinking here.
> 
> (1).Block file system are counting space with blocks. Then the quota
> could works in (dquot_alloc_block & i_blocks) way. I mean, account
> spaces by dquot_alloc_block() and FIOQSIZE can get the qsize from
> i_blocks.
> 
> (2). But ubifs has no blocks, then I choose another way to do it,
> (quot_alloc_space & i_size). That means, we account quota spaces
> in ubifs by dquot_alloc_space() and want FIOSIZE to get i_size
> of inodes. Then there is no notion of *block* but only space.
> 
> So, I want to make FIOSIZE more flexible here to introduce a get_qsize()
> into inode_operations.

But when you use dquot_alloc_space(), then i_blocks / i_bytes will be
updated accordingly by quota code (after all, those values are used when
inode gets transferred between users on chown to update quota information
accordingly). So I don't see a reason why you should need to use i_size in
FIOSIZE.

As a side note, I think that using inode size as the amount of space used
is wrong. I believe even ubifs has a notion of how many bytes of storage
the file occupies and *this* information should be fed into quota subsystem
and updated via dquot_alloc_space(). That way it will be more or less
consistent with how quota for other filesystems works as well. However this
is more or less filesystem internal decision so I can live with ubifs doing
it either way...

								Honza
Yang Dongsheng Sept. 18, 2015, 6:14 a.m. UTC | #4
On 09/17/2015 08:00 PM, Jan Kara wrote:
> On Thu 17-09-15 15:23:11, Dongsheng Yang wrote:
>> On 09/16/2015 06:00 PM, Jan Kara wrote:
>>> On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
>>>> We only care the size of regular file in ubifs for quota.
>>>> The reason is similar with the comment in ubifs_getattr().
>>>>
>>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>>>> ---
>>>>   fs/ubifs/dir.c   |  3 +++
>>>>   fs/ubifs/file.c  | 22 ++++++++++++++++++++++
>>>>   fs/ubifs/ubifs.h |  1 +
>>>>   3 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>>> index 802c6ad..0d3d6d3 100644
>>>> --- a/fs/ubifs/dir.c
>>>> +++ b/fs/ubifs/dir.c
>>>> @@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
>>>>   #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>>   	.update_time = ubifs_update_time,
>>>>   #endif
>>>> +#ifdef CONFIG_QUOTA
>>>> +	.get_qsize	= ubifs_get_qsize,
>>>> +#endif
>>>>   };
>>>>
>>>>   const struct file_operations ubifs_dir_operations = {
>>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>>> index b57ccf3..f1d792a 100644
>>>> --- a/fs/ubifs/file.c
>>>> +++ b/fs/ubifs/file.c
>>>> @@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
>>>>   	return 0;
>>>>   }
>>>>
>>>> +/*
>>>> + * ubifs_get_qsize: get the quota size of a file
>>>> + * @inode: inode which we are going to get the qsize
>>>> + *
>>>> + * We only care the size of regular file in ubifs
>>>> + * for quota. The reason is similar with the comment
>>>> + * in ubifs_getattr().
>>>> + */
>>>> +ssize_t ubifs_get_qsize(struct inode *inode)
>>>> +{
>>>> +	if (S_ISREG(inode->i_mode))
>>>> +		return i_size_read(inode);
>>>> +	else
>>>> +		return 0;
>>>> +}
>>>> +
>>>
>>> The quota space is accounted in bytes. So why don't you store appropriate
>>> number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
>>> have files occupying only say 100 bytes and everything works properly
>>> there so I don't see why ubifs needs to differ.
>>
>> Ha, yes, we did not keep i_blocks in ubifs currently. Because we have
>> no blocks in ubifs. Although we can simulate a i_block for quota, I
>> did not do it. Let me try to show what I am thinking here.
>>
>> (1).Block file system are counting space with blocks. Then the quota
>> could works in (dquot_alloc_block & i_blocks) way. I mean, account
>> spaces by dquot_alloc_block() and FIOQSIZE can get the qsize from
>> i_blocks.
>>
>> (2). But ubifs has no blocks, then I choose another way to do it,
>> (quot_alloc_space & i_size). That means, we account quota spaces
>> in ubifs by dquot_alloc_space() and want FIOSIZE to get i_size
>> of inodes. Then there is no notion of *block* but only space.
>>
>> So, I want to make FIOSIZE more flexible here to introduce a get_qsize()
>> into inode_operations.
>
> But when you use dquot_alloc_space(), then i_blocks / i_bytes will be
> updated accordingly by quota code (after all, those values are used when
> inode gets transferred between users on chown to update quota information
> accordingly). So I don't see a reason why you should need to use i_size in
> FIOSIZE.

Yes, dquot_alloc_space() would update i_blocks. But ubifs_iget() would
never set i_blocks. So i_blocks would be 0 if you get inode from flash
again. Yes, I can make ubifs to maintain a i_blocks. But.....
>
> As a side note, I think that using inode size as the amount of space used
> is wrong. I believe even ubifs has a notion of how many bytes of storage
> the file occupies and *this* information should be fed into quota subsystem
> and updated via dquot_alloc_space(). That way it will be more or less
> consistent with how quota for other filesystems works as well.

.... TBH, there is a little different with other filesystems. I did not
use the "disk" space, but the file space in ubifs quota, although dquot
means disk quota. Same with btrfs quota. If we use disk space for quota,
the most common problem from user is that: why I did not reach the limit
but I can not write any byte. COW in btrfs or out-place-updating in
ubifs makes this problem much worse.

So I choose file space here for ubifs.

Yang
> However this is more or less filesystem internal decision so I can live with ubifs doing
> it either way...
>
> 								Honza
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Sept. 18, 2015, 11:20 a.m. UTC | #5
On Fri 18-09-15 14:14:48, Dongsheng Yang wrote:
> On 09/17/2015 08:00 PM, Jan Kara wrote:
> >On Thu 17-09-15 15:23:11, Dongsheng Yang wrote:
> >>On 09/16/2015 06:00 PM, Jan Kara wrote:
> >>>On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
> >>>>We only care the size of regular file in ubifs for quota.
> >>>>The reason is similar with the comment in ubifs_getattr().
> >>>>
> >>>>Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> >>>>---
> >>>>  fs/ubifs/dir.c   |  3 +++
> >>>>  fs/ubifs/file.c  | 22 ++++++++++++++++++++++
> >>>>  fs/ubifs/ubifs.h |  1 +
> >>>>  3 files changed, 26 insertions(+)
> >>>>
> >>>>diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> >>>>index 802c6ad..0d3d6d3 100644
> >>>>--- a/fs/ubifs/dir.c
> >>>>+++ b/fs/ubifs/dir.c
> >>>>@@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
> >>>>  #ifdef CONFIG_UBIFS_ATIME_SUPPORT
> >>>>  	.update_time = ubifs_update_time,
> >>>>  #endif
> >>>>+#ifdef CONFIG_QUOTA
> >>>>+	.get_qsize	= ubifs_get_qsize,
> >>>>+#endif
> >>>>  };
> >>>>
> >>>>  const struct file_operations ubifs_dir_operations = {
> >>>>diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> >>>>index b57ccf3..f1d792a 100644
> >>>>--- a/fs/ubifs/file.c
> >>>>+++ b/fs/ubifs/file.c
> >>>>@@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
> >>>>  	return 0;
> >>>>  }
> >>>>
> >>>>+/*
> >>>>+ * ubifs_get_qsize: get the quota size of a file
> >>>>+ * @inode: inode which we are going to get the qsize
> >>>>+ *
> >>>>+ * We only care the size of regular file in ubifs
> >>>>+ * for quota. The reason is similar with the comment
> >>>>+ * in ubifs_getattr().
> >>>>+ */
> >>>>+ssize_t ubifs_get_qsize(struct inode *inode)
> >>>>+{
> >>>>+	if (S_ISREG(inode->i_mode))
> >>>>+		return i_size_read(inode);
> >>>>+	else
> >>>>+		return 0;
> >>>>+}
> >>>>+
> >>>
> >>>The quota space is accounted in bytes. So why don't you store appropriate
> >>>number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
> >>>have files occupying only say 100 bytes and everything works properly
> >>>there so I don't see why ubifs needs to differ.
> >>
> >>Ha, yes, we did not keep i_blocks in ubifs currently. Because we have
> >>no blocks in ubifs. Although we can simulate a i_block for quota, I
> >>did not do it. Let me try to show what I am thinking here.
> >>
> >>(1).Block file system are counting space with blocks. Then the quota
> >>could works in (dquot_alloc_block & i_blocks) way. I mean, account
> >>spaces by dquot_alloc_block() and FIOQSIZE can get the qsize from
> >>i_blocks.
> >>
> >>(2). But ubifs has no blocks, then I choose another way to do it,
> >>(quot_alloc_space & i_size). That means, we account quota spaces
> >>in ubifs by dquot_alloc_space() and want FIOSIZE to get i_size
> >>of inodes. Then there is no notion of *block* but only space.
> >>
> >>So, I want to make FIOSIZE more flexible here to introduce a get_qsize()
> >>into inode_operations.
> >
> >But when you use dquot_alloc_space(), then i_blocks / i_bytes will be
> >updated accordingly by quota code (after all, those values are used when
> >inode gets transferred between users on chown to update quota information
> >accordingly). So I don't see a reason why you should need to use i_size in
> >FIOSIZE.
> 
> Yes, dquot_alloc_space() would update i_blocks. But ubifs_iget() would
> never set i_blocks. So i_blocks would be 0 if you get inode from flash
> again. Yes, I can make ubifs to maintain a i_blocks. But.....
> >
> >As a side note, I think that using inode size as the amount of space used
> >is wrong. I believe even ubifs has a notion of how many bytes of storage
> >the file occupies and *this* information should be fed into quota subsystem
> >and updated via dquot_alloc_space(). That way it will be more or less
> >consistent with how quota for other filesystems works as well.
> 
> .... TBH, there is a little different with other filesystems. I did not
> use the "disk" space, but the file space in ubifs quota, although dquot
> means disk quota. Same with btrfs quota. If we use disk space for quota,
> the most common problem from user is that: why I did not reach the limit
> but I can not write any byte. COW in btrfs or out-place-updating in
> ubifs makes this problem much worse.
> 
> So I choose file space here for ubifs.

OK, so these are really two separate questions. I understand your choice of
using file space as the amount of space to account for quota purposes and
I'm fine with that choice. Another thing is that regardless of how you
decide to do quota accounting, you must maintain i_blocks / i_bytes to
contain proper value because dquot_transfer() uses that information to update
quota usage when inode owner is changed.

								Honza
Yang Dongsheng Sept. 21, 2015, 4:35 a.m. UTC | #6
On 09/18/2015 07:20 PM, Jan Kara wrote:
> On Fri 18-09-15 14:14:48, Dongsheng Yang wrote:
>> On 09/17/2015 08:00 PM, Jan Kara wrote:
>>> On Thu 17-09-15 15:23:11, Dongsheng Yang wrote:
>>>> On 09/16/2015 06:00 PM, Jan Kara wrote:
>>>>> On Tue 15-09-15 17:02:31, Dongsheng Yang wrote:
>>>>>> We only care the size of regular file in ubifs for quota.
>>>>>> The reason is similar with the comment in ubifs_getattr().
>>>>>>
>>>>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>   fs/ubifs/dir.c   |  3 +++
>>>>>>   fs/ubifs/file.c  | 22 ++++++++++++++++++++++
>>>>>>   fs/ubifs/ubifs.h |  1 +
>>>>>>   3 files changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>>>>>> index 802c6ad..0d3d6d3 100644
>>>>>> --- a/fs/ubifs/dir.c
>>>>>> +++ b/fs/ubifs/dir.c
>>>>>> @@ -1205,6 +1205,9 @@ const struct inode_operations ubifs_dir_inode_operations = {
>>>>>>   #ifdef CONFIG_UBIFS_ATIME_SUPPORT
>>>>>>   	.update_time = ubifs_update_time,
>>>>>>   #endif
>>>>>> +#ifdef CONFIG_QUOTA
>>>>>> +	.get_qsize	= ubifs_get_qsize,
>>>>>> +#endif
>>>>>>   };
>>>>>>
>>>>>>   const struct file_operations ubifs_dir_operations = {
>>>>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>>>>> index b57ccf3..f1d792a 100644
>>>>>> --- a/fs/ubifs/file.c
>>>>>> +++ b/fs/ubifs/file.c
>>>>>> @@ -1636,6 +1636,22 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
>>>>>>   	return 0;
>>>>>>   }
>>>>>>
>>>>>> +/*
>>>>>> + * ubifs_get_qsize: get the quota size of a file
>>>>>> + * @inode: inode which we are going to get the qsize
>>>>>> + *
>>>>>> + * We only care the size of regular file in ubifs
>>>>>> + * for quota. The reason is similar with the comment
>>>>>> + * in ubifs_getattr().
>>>>>> + */
>>>>>> +ssize_t ubifs_get_qsize(struct inode *inode)
>>>>>> +{
>>>>>> +	if (S_ISREG(inode->i_mode))
>>>>>> +		return i_size_read(inode);
>>>>>> +	else
>>>>>> +		return 0;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> The quota space is accounted in bytes. So why don't you store appropriate
>>>>> number of bytes the file consumes in i_blocks / i_bytes? Reiserfs can also
>>>>> have files occupying only say 100 bytes and everything works properly
>>>>> there so I don't see why ubifs needs to differ.
>>>>
>>>> Ha, yes, we did not keep i_blocks in ubifs currently. Because we have
>>>> no blocks in ubifs. Although we can simulate a i_block for quota, I
>>>> did not do it. Let me try to show what I am thinking here.
>>>>
>>>> (1).Block file system are counting space with blocks. Then the quota
>>>> could works in (dquot_alloc_block & i_blocks) way. I mean, account
>>>> spaces by dquot_alloc_block() and FIOQSIZE can get the qsize from
>>>> i_blocks.
>>>>
>>>> (2). But ubifs has no blocks, then I choose another way to do it,
>>>> (quot_alloc_space & i_size). That means, we account quota spaces
>>>> in ubifs by dquot_alloc_space() and want FIOSIZE to get i_size
>>>> of inodes. Then there is no notion of *block* but only space.
>>>>
>>>> So, I want to make FIOSIZE more flexible here to introduce a get_qsize()
>>>> into inode_operations.
>>>
>>> But when you use dquot_alloc_space(), then i_blocks / i_bytes will be
>>> updated accordingly by quota code (after all, those values are used when
>>> inode gets transferred between users on chown to update quota information
>>> accordingly). So I don't see a reason why you should need to use i_size in
>>> FIOSIZE.
>>
>> Yes, dquot_alloc_space() would update i_blocks. But ubifs_iget() would
>> never set i_blocks. So i_blocks would be 0 if you get inode from flash
>> again. Yes, I can make ubifs to maintain a i_blocks. But.....
>>>
>>> As a side note, I think that using inode size as the amount of space used
>>> is wrong. I believe even ubifs has a notion of how many bytes of storage
>>> the file occupies and *this* information should be fed into quota subsystem
>>> and updated via dquot_alloc_space(). That way it will be more or less
>>> consistent with how quota for other filesystems works as well.
>>
>> .... TBH, there is a little different with other filesystems. I did not
>> use the "disk" space, but the file space in ubifs quota, although dquot
>> means disk quota. Same with btrfs quota. If we use disk space for quota,
>> the most common problem from user is that: why I did not reach the limit
>> but I can not write any byte. COW in btrfs or out-place-updating in
>> ubifs makes this problem much worse.
>>
>> So I choose file space here for ubifs.
>
> OK, so these are really two separate questions. I understand your choice of
> using file space as the amount of space to account for quota purposes and
> I'm fine with that choice. Another thing is that regardless of how you
> decide to do quota accounting, you must maintain i_blocks / i_bytes to
> contain proper value because dquot_transfer() uses that information to update
> quota usage when inode owner is changed.

But if we don't use i_blocks to get qsize, what we care only in 
dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
is not correct in dquot_transfer() in ubifs, that's okey, because we
will never use this value, right?

Yang
>
> 								Honza
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Sept. 21, 2015, 9:13 a.m. UTC | #7
On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
> On 09/18/2015 07:20 PM, Jan Kara wrote:
> >>.... TBH, there is a little different with other filesystems. I did not
> >>use the "disk" space, but the file space in ubifs quota, although dquot
> >>means disk quota. Same with btrfs quota. If we use disk space for quota,
> >>the most common problem from user is that: why I did not reach the limit
> >>but I can not write any byte. COW in btrfs or out-place-updating in
> >>ubifs makes this problem much worse.
> >>
> >>So I choose file space here for ubifs.
> >
> >OK, so these are really two separate questions. I understand your choice of
> >using file space as the amount of space to account for quota purposes and
> >I'm fine with that choice. Another thing is that regardless of how you
> >decide to do quota accounting, you must maintain i_blocks / i_bytes to
> >contain proper value because dquot_transfer() uses that information to update
> >quota usage when inode owner is changed.
> 
> But if we don't use i_blocks to get qsize, what we care only in
> dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
> is not correct in dquot_transfer() in ubifs, that's okey, because we
> will never use this value, right?

dquot_transfer() will use the value - when file F changes owner from user A
to user B, then you need to decrement amount of space used by F from A's
quota usage and add that amount to B's quota usage. And the amount of space
is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
__dquot_transfer() in fs/quota/dquot.c for details.

								Honza
Yang Dongsheng Sept. 21, 2015, 9:16 a.m. UTC | #8
On 09/21/2015 05:13 PM, Jan Kara wrote:
> On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
>> On 09/18/2015 07:20 PM, Jan Kara wrote:
>>>> .... TBH, there is a little different with other filesystems. I did not
>>>> use the "disk" space, but the file space in ubifs quota, although dquot
>>>> means disk quota. Same with btrfs quota. If we use disk space for quota,
>>>> the most common problem from user is that: why I did not reach the limit
>>>> but I can not write any byte. COW in btrfs or out-place-updating in
>>>> ubifs makes this problem much worse.
>>>>
>>>> So I choose file space here for ubifs.
>>>
>>> OK, so these are really two separate questions. I understand your choice of
>>> using file space as the amount of space to account for quota purposes and
>>> I'm fine with that choice. Another thing is that regardless of how you
>>> decide to do quota accounting, you must maintain i_blocks / i_bytes to
>>> contain proper value because dquot_transfer() uses that information to update
>>> quota usage when inode owner is changed.
>>
>> But if we don't use i_blocks to get qsize, what we care only in
>> dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
>> is not correct in dquot_transfer() in ubifs, that's okey, because we
>> will never use this value, right?
>
> dquot_transfer() will use the value - when file F changes owner from user A
> to user B, then you need to decrement amount of space used by F from A's
> quota usage and add that amount to B's quota usage. And the amount of space
> is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
> __dquot_transfer() in fs/quota/dquot.c for details.

Yes, I see it. But if ubifs doesn't use i_blocks and i_bytes to stand
for quota size, as I mentioned I want to use i_size. Then we will never
use i_blocks. So we only need a to transfer dquot->dq_dqb values. That
means, even the i_blocks in dquot_transfer() is not correct, we don't
care it. we only need to make sure values in dquot_dq_dqb are 
transfered, such as dquot->dq_dqb->curspace for user B is equal with
i_size.

Yang


>
> 								Honza
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Sept. 21, 2015, 9:44 a.m. UTC | #9
On Mon 21-09-15 17:16:49, Dongsheng Yang wrote:
> On 09/21/2015 05:13 PM, Jan Kara wrote:
> >On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
> >>On 09/18/2015 07:20 PM, Jan Kara wrote:
> >>>>.... TBH, there is a little different with other filesystems. I did not
> >>>>use the "disk" space, but the file space in ubifs quota, although dquot
> >>>>means disk quota. Same with btrfs quota. If we use disk space for quota,
> >>>>the most common problem from user is that: why I did not reach the limit
> >>>>but I can not write any byte. COW in btrfs or out-place-updating in
> >>>>ubifs makes this problem much worse.
> >>>>
> >>>>So I choose file space here for ubifs.
> >>>
> >>>OK, so these are really two separate questions. I understand your choice of
> >>>using file space as the amount of space to account for quota purposes and
> >>>I'm fine with that choice. Another thing is that regardless of how you
> >>>decide to do quota accounting, you must maintain i_blocks / i_bytes to
> >>>contain proper value because dquot_transfer() uses that information to update
> >>>quota usage when inode owner is changed.
> >>
> >>But if we don't use i_blocks to get qsize, what we care only in
> >>dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
> >>is not correct in dquot_transfer() in ubifs, that's okey, because we
> >>will never use this value, right?
> >
> >dquot_transfer() will use the value - when file F changes owner from user A
> >to user B, then you need to decrement amount of space used by F from A's
> >quota usage and add that amount to B's quota usage. And the amount of space
> >is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
> >__dquot_transfer() in fs/quota/dquot.c for details.
> 
> Yes, I see it. But if ubifs doesn't use i_blocks and i_bytes to stand
> for quota size, as I mentioned I want to use i_size. Then we will never
> use i_blocks. So we only need a to transfer dquot->dq_dqb values. That
> means, even the i_blocks in dquot_transfer() is not correct, we don't
> care it. we only need to make sure values in dquot_dq_dqb are
> transfered, such as dquot->dq_dqb->curspace for user B is equal with
> i_size.

I think you are missing one thing:

Assume user A has files F1 (1MB) F2 (2MB) F3 (3MB).
User B has one file G1 (4MB).

So user A uses 6 MB in total (stored in dquot for user A in dqb_curspace
field). User B uses 4MB.

Now you do "chown B F1"

You need to change dq_dqb of F1 to point to user B quota structure as you
explain, that is correct. However you also need to subtract 1MB from user
A's total usage (which is stored in struct dquot for user A) since file F1
no longer belongs to him. And you need to add 1 MB to user B's total usage.

After this user A will have total usage of 5 MB and user B has usage of 5
MB as well. You need to properly update the total usage of each user so
that you can check the usage against quota limits.

								Honza
Yang Dongsheng Sept. 21, 2015, 11:02 a.m. UTC | #10
On 09/21/2015 05:44 PM, Jan Kara wrote:
> On Mon 21-09-15 17:16:49, Dongsheng Yang wrote:
>> On 09/21/2015 05:13 PM, Jan Kara wrote:
>>> On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
>>>> On 09/18/2015 07:20 PM, Jan Kara wrote:
>>>>>> .... TBH, there is a little different with other filesystems. I did not
>>>>>> use the "disk" space, but the file space in ubifs quota, although dquot
>>>>>> means disk quota. Same with btrfs quota. If we use disk space for quota,
>>>>>> the most common problem from user is that: why I did not reach the limit
>>>>>> but I can not write any byte. COW in btrfs or out-place-updating in
>>>>>> ubifs makes this problem much worse.
>>>>>>
>>>>>> So I choose file space here for ubifs.
>>>>>
>>>>> OK, so these are really two separate questions. I understand your choice of
>>>>> using file space as the amount of space to account for quota purposes and
>>>>> I'm fine with that choice. Another thing is that regardless of how you
>>>>> decide to do quota accounting, you must maintain i_blocks / i_bytes to
>>>>> contain proper value because dquot_transfer() uses that information to update
>>>>> quota usage when inode owner is changed.
>>>>
>>>> But if we don't use i_blocks to get qsize, what we care only in
>>>> dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
>>>> is not correct in dquot_transfer() in ubifs, that's okey, because we
>>>> will never use this value, right?
>>>
>>> dquot_transfer() will use the value - when file F changes owner from user A
>>> to user B, then you need to decrement amount of space used by F from A's
>>> quota usage and add that amount to B's quota usage. And the amount of space
>>> is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
>>> __dquot_transfer() in fs/quota/dquot.c for details.
>>
>> Yes, I see it. But if ubifs doesn't use i_blocks and i_bytes to stand
>> for quota size, as I mentioned I want to use i_size. Then we will never
>> use i_blocks. So we only need a to transfer dquot->dq_dqb values. That
>> means, even the i_blocks in dquot_transfer() is not correct, we don't
>> care it. we only need to make sure values in dquot_dq_dqb are
>> transfered, such as dquot->dq_dqb->curspace for user B is equal with
>> i_size.
>
> I think you are missing one thing:
>
> Assume user A has files F1 (1MB) F2 (2MB) F3 (3MB).
> User B has one file G1 (4MB).
>
> So user A uses 6 MB in total (stored in dquot for user A in dqb_curspace
> field). User B uses 4MB.
>
> Now you do "chown B F1"
>
> You need to change dq_dqb of F1 to point to user B quota structure as you
> explain, that is correct. However you also need to subtract 1MB from user
> A's total usage (which is stored in struct dquot for user A) since file F1
> no longer belongs to him. And you need to add 1 MB to user B's total usage.
>
> After this user A will have total usage of 5 MB and user B has usage of 5
> MB as well. You need to properly update the total usage of each user so
> that you can check the usage against quota limits.

But that's all done in dquot_transfer(). It dquot_decr_space(from) and
dquot_incr_space(to). Let me show the test result in my implementation.

(1). root -> 1M/2M/3M     yds ->4M

[root@atest-guest linux_compile]# ll -h /mnt/ubifs/
total 11M
-rw-r--r--. 1 root root 1.0M Sep 21 15:54 1M
-rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
-rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
-rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
-rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
-rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
[root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
*** Report for user quotas on device /dev/ubi0_0
Block grace time: 7days; Inode grace time: 7days
                         Block limits                File limits
User            used    soft    hard  grace    used  soft  hard  grace
----------------------------------------------------------------------
root      --    6144       0       0              4     0     0
yds       --    4096       0       0              1     0     0


(2). chown yds /mnt/ubifs/1M
[root@atest-guest linux_compile]# chown yds /mnt/ubifs/1M
[root@atest-guest linux_compile]# ll -h /mnt/ubifs/
total 11M
-rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
-rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
-rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
-rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
-rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
-rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
[root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
*** Report for user quotas on device /dev/ubi0_0
Block grace time: 7days; Inode grace time: 7days
                         Block limits                File limits
User            used    soft    hard  grace    used  soft  hard  grace
----------------------------------------------------------------------
root      --    5120       0       0              3     0     0
yds       --    5120       0       0              2     0     0


(3). quotacheck and check again.
[root@atest-guest linux_compile]# quotaoff /mnt/ubifs && quotacheck -u 
/mnt/ubifs/
[root@atest-guest linux_compile]# ll -h /mnt/ubifs
total 11M
-rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
-rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
-rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
-rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
-rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
-rw-------. 1 root root 7.0K Sep 21 16:00 aquota.user
[root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
*** Report for user quotas on device /dev/ubi0_0
Block grace time: 7days; Inode grace time: 7days
                         Block limits                File limits
User            used    soft    hard  grace    used  soft  hard  grace
----------------------------------------------------------------------
root      --    5120       0       0              3     0     0
yds       --    5120       0       0              2     0     0

Yang
>
> 								Honza
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Sept. 23, 2015, 7:42 a.m. UTC | #11
On Mon 21-09-15 19:02:30, Dongsheng Yang wrote:
> On 09/21/2015 05:44 PM, Jan Kara wrote:
> >On Mon 21-09-15 17:16:49, Dongsheng Yang wrote:
> >>On 09/21/2015 05:13 PM, Jan Kara wrote:
> >>>On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
> >>>>On 09/18/2015 07:20 PM, Jan Kara wrote:
> >>>>>>.... TBH, there is a little different with other filesystems. I did not
> >>>>>>use the "disk" space, but the file space in ubifs quota, although dquot
> >>>>>>means disk quota. Same with btrfs quota. If we use disk space for quota,
> >>>>>>the most common problem from user is that: why I did not reach the limit
> >>>>>>but I can not write any byte. COW in btrfs or out-place-updating in
> >>>>>>ubifs makes this problem much worse.
> >>>>>>
> >>>>>>So I choose file space here for ubifs.
> >>>>>
> >>>>>OK, so these are really two separate questions. I understand your choice of
> >>>>>using file space as the amount of space to account for quota purposes and
> >>>>>I'm fine with that choice. Another thing is that regardless of how you
> >>>>>decide to do quota accounting, you must maintain i_blocks / i_bytes to
> >>>>>contain proper value because dquot_transfer() uses that information to update
> >>>>>quota usage when inode owner is changed.
> >>>>
> >>>>But if we don't use i_blocks to get qsize, what we care only in
> >>>>dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
> >>>>is not correct in dquot_transfer() in ubifs, that's okey, because we
> >>>>will never use this value, right?
> >>>
> >>>dquot_transfer() will use the value - when file F changes owner from user A
> >>>to user B, then you need to decrement amount of space used by F from A's
> >>>quota usage and add that amount to B's quota usage. And the amount of space
> >>>is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
> >>>__dquot_transfer() in fs/quota/dquot.c for details.
> >>
> >>Yes, I see it. But if ubifs doesn't use i_blocks and i_bytes to stand
> >>for quota size, as I mentioned I want to use i_size. Then we will never
> >>use i_blocks. So we only need a to transfer dquot->dq_dqb values. That
> >>means, even the i_blocks in dquot_transfer() is not correct, we don't
> >>care it. we only need to make sure values in dquot_dq_dqb are
> >>transfered, such as dquot->dq_dqb->curspace for user B is equal with
> >>i_size.
> >
> >I think you are missing one thing:
> >
> >Assume user A has files F1 (1MB) F2 (2MB) F3 (3MB).
> >User B has one file G1 (4MB).
> >
> >So user A uses 6 MB in total (stored in dquot for user A in dqb_curspace
> >field). User B uses 4MB.
> >
> >Now you do "chown B F1"
> >
> >You need to change dq_dqb of F1 to point to user B quota structure as you
> >explain, that is correct. However you also need to subtract 1MB from user
> >A's total usage (which is stored in struct dquot for user A) since file F1
> >no longer belongs to him. And you need to add 1 MB to user B's total usage.
> >
> >After this user A will have total usage of 5 MB and user B has usage of 5
> >MB as well. You need to properly update the total usage of each user so
> >that you can check the usage against quota limits.
> 
> But that's all done in dquot_transfer(). It dquot_decr_space(from) and
> dquot_incr_space(to). Let me show the test result in my implementation.

Yes, dquot_decr_space(transfer_from[cnt], cur_space) and
dquot_incr_space(transfer_to[cnt], cur_space) are the functions changing
usage. However have a look at what cur_space is. It is set as:

cur_space = inode_get_bytes(inode);

and inode_get_bytes() is:

ret = (((loff_t)inode->i_blocks) << 9) + inode->i_bytes;

So for dquot_transfer() to work correctly, filesystem has to properly
maintain i_blocks and i_bytes.

> (1). root -> 1M/2M/3M     yds ->4M
> 
> [root@atest-guest linux_compile]# ll -h /mnt/ubifs/
> total 11M
> -rw-r--r--. 1 root root 1.0M Sep 21 15:54 1M
> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
> -rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
> *** Report for user quotas on device /dev/ubi0_0
> Block grace time: 7days; Inode grace time: 7days
>                         Block limits                File limits
> User            used    soft    hard  grace    used  soft  hard  grace
> ----------------------------------------------------------------------
> root      --    6144       0       0              4     0     0
> yds       --    4096       0       0              1     0     0
> 
> 
> (2). chown yds /mnt/ubifs/1M
> [root@atest-guest linux_compile]# chown yds /mnt/ubifs/1M
> [root@atest-guest linux_compile]# ll -h /mnt/ubifs/
> total 11M
> -rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
> -rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
> *** Report for user quotas on device /dev/ubi0_0
> Block grace time: 7days; Inode grace time: 7days
>                         Block limits                File limits
> User            used    soft    hard  grace    used  soft  hard  grace
> ----------------------------------------------------------------------
> root      --    5120       0       0              3     0     0
> yds       --    5120       0       0              2     0     0
> 
> 
> (3). quotacheck and check again.
> [root@atest-guest linux_compile]# quotaoff /mnt/ubifs && quotacheck
> -u /mnt/ubifs/
> [root@atest-guest linux_compile]# ll -h /mnt/ubifs
> total 11M
> -rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
> -rw-------. 1 root root 7.0K Sep 21 16:00 aquota.user
> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
> *** Report for user quotas on device /dev/ubi0_0
> Block grace time: 7days; Inode grace time: 7days
>                         Block limits                File limits
> User            used    soft    hard  grace    used  soft  hard  grace
> ----------------------------------------------------------------------
> root      --    5120       0       0              3     0     0
> yds       --    5120       0       0              2     0     0

Yes, this case is working because quota code updated i_blocks properly when
creating files. But if you do not set i_blocks when reading inodes
(which seems to be the case) I expect the following would not work:

mount -t ubifs -o usrjquota=aquota.user,jqfmt=vfsv0 <dev> /mnt
quotacheck -vu /mnt
quotaon -vu /mnt
dd if=/dev/zero of=file1 bs=1M count=1
# Umount fs to prune cached information
umount /mnt
mount -t ubifs -o usrjquota=aquota.user,jqfmt=vfsv0 <dev> /mnt
quotaon -vu /mnt
chown yds /mnt/file1
repquota -u /mnt/

Now unless you set i_blocks when reading inode, yds would not be accounted
for 1 MB /mnt/file1 is using...

								Honza
Yang Dongsheng Sept. 24, 2015, 12:50 a.m. UTC | #12
On 09/23/2015 03:42 PM, Jan Kara wrote:
> On Mon 21-09-15 19:02:30, Dongsheng Yang wrote:
>> On 09/21/2015 05:44 PM, Jan Kara wrote:
>>> On Mon 21-09-15 17:16:49, Dongsheng Yang wrote:
>>>> On 09/21/2015 05:13 PM, Jan Kara wrote:
>>>>> On Mon 21-09-15 12:35:01, Dongsheng Yang wrote:
>>>>>> On 09/18/2015 07:20 PM, Jan Kara wrote:
>>>>>>>> .... TBH, there is a little different with other filesystems. I did not
>>>>>>>> use the "disk" space, but the file space in ubifs quota, although dquot
>>>>>>>> means disk quota. Same with btrfs quota. If we use disk space for quota,
>>>>>>>> the most common problem from user is that: why I did not reach the limit
>>>>>>>> but I can not write any byte. COW in btrfs or out-place-updating in
>>>>>>>> ubifs makes this problem much worse.
>>>>>>>>
>>>>>>>> So I choose file space here for ubifs.
>>>>>>>
>>>>>>> OK, so these are really two separate questions. I understand your choice of
>>>>>>> using file space as the amount of space to account for quota purposes and
>>>>>>> I'm fine with that choice. Another thing is that regardless of how you
>>>>>>> decide to do quota accounting, you must maintain i_blocks / i_bytes to
>>>>>>> contain proper value because dquot_transfer() uses that information to update
>>>>>>> quota usage when inode owner is changed.
>>>>>>
>>>>>> But if we don't use i_blocks to get qsize, what we care only in
>>>>>> dquot_transter() is dquot->dq_dqb. That means, even if the i_blocks
>>>>>> is not correct in dquot_transfer() in ubifs, that's okey, because we
>>>>>> will never use this value, right?
>>>>>
>>>>> dquot_transfer() will use the value - when file F changes owner from user A
>>>>> to user B, then you need to decrement amount of space used by F from A's
>>>>> quota usage and add that amount to B's quota usage. And the amount of space
>>>>> is obtained via inode_get_bytes() which uses i_blocks and i_bytes. See
>>>>> __dquot_transfer() in fs/quota/dquot.c for details.
>>>>
>>>> Yes, I see it. But if ubifs doesn't use i_blocks and i_bytes to stand
>>>> for quota size, as I mentioned I want to use i_size. Then we will never
>>>> use i_blocks. So we only need a to transfer dquot->dq_dqb values. That
>>>> means, even the i_blocks in dquot_transfer() is not correct, we don't
>>>> care it. we only need to make sure values in dquot_dq_dqb are
>>>> transfered, such as dquot->dq_dqb->curspace for user B is equal with
>>>> i_size.
>>>
>>> I think you are missing one thing:
>>>
>>> Assume user A has files F1 (1MB) F2 (2MB) F3 (3MB).
>>> User B has one file G1 (4MB).
>>>
>>> So user A uses 6 MB in total (stored in dquot for user A in dqb_curspace
>>> field). User B uses 4MB.
>>>
>>> Now you do "chown B F1"
>>>
>>> You need to change dq_dqb of F1 to point to user B quota structure as you
>>> explain, that is correct. However you also need to subtract 1MB from user
>>> A's total usage (which is stored in struct dquot for user A) since file F1
>>> no longer belongs to him. And you need to add 1 MB to user B's total usage.
>>>
>>> After this user A will have total usage of 5 MB and user B has usage of 5
>>> MB as well. You need to properly update the total usage of each user so
>>> that you can check the usage against quota limits.
>>
>> But that's all done in dquot_transfer(). It dquot_decr_space(from) and
>> dquot_incr_space(to). Let me show the test result in my implementation.
>
> Yes, dquot_decr_space(transfer_from[cnt], cur_space) and
> dquot_incr_space(transfer_to[cnt], cur_space) are the functions changing
> usage. However have a look at what cur_space is. It is set as:
>
> cur_space = inode_get_bytes(inode);
>
> and inode_get_bytes() is:
>
> ret = (((loff_t)inode->i_blocks) << 9) + inode->i_bytes;
>
> So for dquot_transfer() to work correctly, filesystem has to properly
> maintain i_blocks and i_bytes.

Oh, my mistake. you are right. We have to maintain i_blocks for quota
then. will update it.

Yang
>
>> (1). root -> 1M/2M/3M     yds ->4M
>>
>> [root@atest-guest linux_compile]# ll -h /mnt/ubifs/
>> total 11M
>> -rw-r--r--. 1 root root 1.0M Sep 21 15:54 1M
>> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
>> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
>> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
>> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
>> -rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
>> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
>> *** Report for user quotas on device /dev/ubi0_0
>> Block grace time: 7days; Inode grace time: 7days
>>                          Block limits                File limits
>> User            used    soft    hard  grace    used  soft  hard  grace
>> ----------------------------------------------------------------------
>> root      --    6144       0       0              4     0     0
>> yds       --    4096       0       0              1     0     0
>>
>>
>> (2). chown yds /mnt/ubifs/1M
>> [root@atest-guest linux_compile]# chown yds /mnt/ubifs/1M
>> [root@atest-guest linux_compile]# ll -h /mnt/ubifs/
>> total 11M
>> -rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
>> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
>> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
>> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
>> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
>> -rw-------. 1 root root 7.0K Sep 21 15:53 aquota.user
>> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
>> *** Report for user quotas on device /dev/ubi0_0
>> Block grace time: 7days; Inode grace time: 7days
>>                          Block limits                File limits
>> User            used    soft    hard  grace    used  soft  hard  grace
>> ----------------------------------------------------------------------
>> root      --    5120       0       0              3     0     0
>> yds       --    5120       0       0              2     0     0
>>
>>
>> (3). quotacheck and check again.
>> [root@atest-guest linux_compile]# quotaoff /mnt/ubifs && quotacheck
>> -u /mnt/ubifs/
>> [root@atest-guest linux_compile]# ll -h /mnt/ubifs
>> total 11M
>> -rw-r--r--. 1 yds  root 1.0M Sep 21 15:54 1M
>> -rw-r--r--. 1 root root 2.0M Sep 21 15:54 2M
>> -rw-r--r--. 1 root root 3.0M Sep 21 15:54 3M
>> -rw-r--r--. 1 yds  root 4.0M Sep 21 15:55 4M
>> -rw-------. 1 root root 6.0K Sep 21 15:53 aquota.group
>> -rw-------. 1 root root 7.0K Sep 21 16:00 aquota.user
>> [root@atest-guest linux_compile]# repquota -u /mnt/ubifs/
>> *** Report for user quotas on device /dev/ubi0_0
>> Block grace time: 7days; Inode grace time: 7days
>>                          Block limits                File limits
>> User            used    soft    hard  grace    used  soft  hard  grace
>> ----------------------------------------------------------------------
>> root      --    5120       0       0              3     0     0
>> yds       --    5120       0       0              2     0     0
>
> Yes, this case is working because quota code updated i_blocks properly when
> creating files. But if you do not set i_blocks when reading inodes
> (which seems to be the case) I expect the following would not work:
>
> mount -t ubifs -o usrjquota=aquota.user,jqfmt=vfsv0 <dev> /mnt
> quotacheck -vu /mnt
> quotaon -vu /mnt
> dd if=/dev/zero of=file1 bs=1M count=1
> # Umount fs to prune cached information
> umount /mnt
> mount -t ubifs -o usrjquota=aquota.user,jqfmt=vfsv0 <dev> /mnt
> quotaon -vu /mnt
> chown yds /mnt/file1
> repquota -u /mnt/
>
> Now unless you set i_blocks when reading inode, yds would not be accounted
> for 1 MB /mnt/file1 is using...
>
> 								Honza
>

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 802c6ad..0d3d6d3 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -1205,6 +1205,9 @@  const struct inode_operations ubifs_dir_inode_operations = {
 #ifdef CONFIG_UBIFS_ATIME_SUPPORT
 	.update_time = ubifs_update_time,
 #endif
+#ifdef CONFIG_QUOTA
+	.get_qsize	= ubifs_get_qsize,
+#endif
 };
 
 const struct file_operations ubifs_dir_operations = {
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index b57ccf3..f1d792a 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1636,6 +1636,22 @@  static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	return 0;
 }
 
+/*
+ * ubifs_get_qsize: get the quota size of a file
+ * @inode: inode which we are going to get the qsize
+ *
+ * We only care the size of regular file in ubifs
+ * for quota. The reason is similar with the comment
+ * in ubifs_getattr().
+ */
+ssize_t ubifs_get_qsize(struct inode *inode)
+{
+	if (S_ISREG(inode->i_mode))
+		return i_size_read(inode);
+	else
+		return 0;
+}
+
 const struct address_space_operations ubifs_file_address_operations = {
 	.readpage       = ubifs_readpage,
 	.writepage      = ubifs_writepage,
@@ -1656,6 +1672,9 @@  const struct inode_operations ubifs_file_inode_operations = {
 #ifdef CONFIG_UBIFS_ATIME_SUPPORT
 	.update_time = ubifs_update_time,
 #endif
+#ifdef CONFIG_QUOTA
+	.get_qsize	= ubifs_get_qsize,
+#endif
 };
 
 const struct inode_operations ubifs_symlink_inode_operations = {
@@ -1670,6 +1689,9 @@  const struct inode_operations ubifs_symlink_inode_operations = {
 #ifdef CONFIG_UBIFS_ATIME_SUPPORT
 	.update_time = ubifs_update_time,
 #endif
+#ifdef CONFIG_QUOTA
+	.get_qsize	= ubifs_get_qsize,
+#endif
 };
 
 const struct file_operations ubifs_file_operations = {
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 99cf10c..21b5dc0 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1759,6 +1759,7 @@  int ubifs_read_block(struct inode *inode, void *addr, unsigned int block,
 int ubifs_fsync(struct file *file, loff_t start, loff_t end, int datasync);
 int ubifs_setattr(struct dentry *dentry, struct iattr *attr);
 int ubifs_update_time(struct inode *inode, struct timespec *time, int flags);
+ssize_t ubifs_get_qsize(struct inode *inode);
 
 /* dir.c */
 struct inode *ubifs_new_inode(struct ubifs_info *c, const struct inode *dir,