Re: [Jfs-discussion] [PATCH 28/28] quota: add extra inode count to dquot transfer functions

2017-06-20 Thread Tahsin Erdogan via Jfs-discussion
On Mon, Jun 19, 2017 at 5:36 AM, Jan Kara  wrote:
> Heh, this "pushing of responsibility" looks like a silly game. If an error
> can happen in a function, it is better to report it as far as easily
> possible (unless we can cleanly handle it which we cannot here). I'm guilty
> of making dquot_free_inode() ignore errors from mark_all_dquot_dirty() and
> in retrospect it would have been better if these were propagated to the
> caller as well. And eventually we can fix this if we decide we care enough.
> I'm completely fine with just returning an error from dquot_free_inode()
> and ignore it in all the callers except for ext4. Then filesystems which
> care enough can try to handle the error. That way we at least don't
> increase the design debt from the past.

I sent an update but since patch title changed it landed in a new
email thread I think ("[PATCH v2 28/31] quota: add get_inode_usage
callback to transfer multi-inode charges"). I will respond to your
comment in that thread.

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion


Re: [Jfs-discussion] [PATCH 28/28] quota: add extra inode count to dquot transfer functions

2017-06-19 Thread Jan Kara
On Fri 16-06-17 18:50:58, Tahsin Erdogan wrote:
> On Thu, Jun 15, 2017 at 12:57 AM, Jan Kara  wrote:
> > Hum, rather handle this similarly to how we handle delalloc reserved space.
> > Add a callback to dq_ops to get "inode usage" of an inode and then use it
> > in dquot_transfer(), dquot_free_inode(), dquot_alloc_inode().
> 
> I tried that approach by adding a "int get_inode_usage(struct inode
> *inode, qsize_t *usage)" callback to dquot_operations. Unfortunately,
> ext4 code that calculates the number of internal inodes
> (ext4_xattr_inode_count()) is subject to failures so the callback has
> to be able to report errors. And, that itself is problematic because
> we can't afford to have errors in dquot_free_inode(). If you have
> thoughts about how to address this please let me know.

Well, you can just make dquot_free_inode() return error. Now most callers
won't be able to do much with an error from dquot_free_inode() but that's
the case also for other things during inode deletion - just handle it as
other fatal failures during inode freeing.

> Alternatively, I could try to make this patch less intrusive by
> keeping the existing dquot_transfer() signature and add a new
> dquot_transfer_usage() that accepts inode_usage as a parameter. What
> do you think?

That would be somewhat better than what you do in this patch but I prefer
to handle this like I suggested above.

Honza
-- 
Jan Kara 
SUSE Labs, CR

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion


Re: [Jfs-discussion] [PATCH 28/28] quota: add extra inode count to dquot transfer functions

2017-06-16 Thread Tahsin Erdogan via Jfs-discussion
On Thu, Jun 15, 2017 at 12:57 AM, Jan Kara  wrote:
> Hum, rather handle this similarly to how we handle delalloc reserved space.
> Add a callback to dq_ops to get "inode usage" of an inode and then use it
> in dquot_transfer(), dquot_free_inode(), dquot_alloc_inode().

I tried that approach by adding a "int get_inode_usage(struct inode
*inode, qsize_t *usage)" callback to dquot_operations. Unfortunately,
ext4 code that calculates the number of internal inodes
(ext4_xattr_inode_count()) is subject to failures so the callback has
to be able to report errors. And, that itself is problematic because
we can't afford to have errors in dquot_free_inode(). If you have
thoughts about how to address this please let me know.

Alternatively, I could try to make this patch less intrusive by
keeping the existing dquot_transfer() signature and add a new
dquot_transfer_usage() that accepts inode_usage as a parameter. What
do you think?

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Jfs-discussion mailing list
Jfs-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/jfs-discussion


Re: [Jfs-discussion] [PATCH 28/28] quota: add extra inode count to dquot transfer functions

2017-06-15 Thread Jan Kara
On Wed 31-05-17 01:15:17, Tahsin Erdogan wrote:
> Ext4 ea_inode feature allows storing xattr values in external inodes to
> be able to store values that are bigger than a block in size. Ext4 also
> has deduplication support for these type of inodes. With deduplication,
> the actual storage waste is eliminated but the users of such inodes are
> still charged full quota for the inodes as if there was no sharing
> happening in the background.
> 
> This design requires ext4 to manually charge the users because the
> inodes are shared.
> 
> An implication of this is that, if someone calls chown on a file that
> has such references we need to transfer the quota for the file and xattr
> inodes. Current dquot_transfer() function implicitly transfers one inode
> charge. In our case, we would like to specify additional inodes to be
> transferred.

Hum, rather handle this similarly to how we handle delalloc reserved space.
Add a callback to dq_ops to get "inode usage" of an inode and then use it
in dquot_transfer(), dquot_free_inode(), dquot_alloc_inode().

Honza

 
> Signed-off-by: Tahsin Erdogan 
> ---
>  fs/ext2/inode.c  |  2 +-
>  fs/ext4/inode.c  |  8 ++-
>  fs/ext4/ioctl.c  | 13 +++-
>  fs/ext4/xattr.c  | 54 
> 
>  fs/ext4/xattr.h  |  2 ++
>  fs/jfs/file.c|  2 +-
>  fs/ocfs2/file.c  |  2 +-
>  fs/quota/dquot.c | 16 +++---
>  fs/reiserfs/inode.c  |  2 +-
>  include/linux/quotaops.h |  8 ---
>  10 files changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index 2dcbd5698884..a13ba5dcb355 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1656,7 +1656,7 @@ int ext2_setattr(struct dentry *dentry, struct iattr 
> *iattr)
>   }
>   if ((iattr->ia_valid & ATTR_UID && !uid_eq(iattr->ia_uid, 
> inode->i_uid)) ||
>   (iattr->ia_valid & ATTR_GID && !gid_eq(iattr->ia_gid, 
> inode->i_gid))) {
> - error = dquot_transfer(inode, iattr);
> + error = dquot_transfer(inode, iattr, 0);
>   if (error)
>   return error;
>   }
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6f5872197d6c..28abbbdbbb80 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5267,6 +5267,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr 
> *attr)
>   int error, rc = 0;
>   int orphan = 0;
>   const unsigned int ia_valid = attr->ia_valid;
> + int ea_inode_refs;
>  
>   if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb
>   return -EIO;
> @@ -5293,7 +5294,12 @@ int ext4_setattr(struct dentry *dentry, struct iattr 
> *attr)
>   error = PTR_ERR(handle);
>   goto err_out;
>   }
> - error = dquot_transfer(inode, attr);
> +
> + down_read(_I(inode)->xattr_sem);
> + error = ea_inode_refs = ext4_xattr_inode_count(inode);
> + if (ea_inode_refs >= 0)
> + error = dquot_transfer(inode, attr, ea_inode_refs);
> + up_read(_I(inode)->xattr_sem);
>   if (error) {
>   ext4_journal_stop(handle);
>   return error;
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index dde8deb11e59..9938dc8e24c8 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -21,6 +21,7 @@
>  #include "ext4.h"
>  #include 
>  #include "fsmap.h"
> +#include "xattr.h"
>  #include 
>  
>  /**
> @@ -319,6 +320,7 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 
> projid)
>   struct ext4_iloc iloc;
>   struct ext4_inode *raw_inode;
>   struct dquot *transfer_to[MAXQUOTAS] = { };
> + int ea_inode_refs;
>  
>   if (!ext4_has_feature_project(sb)) {
>   if (projid != EXT4_DEF_PROJID)
> @@ -371,9 +373,17 @@ static int ext4_ioctl_setproject(struct file *filp, 
> __u32 projid)
>   if (err)
>   goto out_stop;
>  
> + down_read(_I(inode)->xattr_sem);
> + ea_inode_refs = ext4_xattr_inode_count(inode);
> + if (ea_inode_refs < 0) {
> + up_read(_I(inode)->xattr_sem);
> + err = ea_inode_refs;
> + goto out_stop;
> + }
> +
>   transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
>   if (!IS_ERR(transfer_to[PRJQUOTA])) {
> - err = __dquot_transfer(inode, transfer_to);
> + err = __dquot_transfer(inode, transfer_to, ea_inode_refs);
>   dqput(transfer_to[PRJQUOTA]);
>   if (err)
>   goto out_dirty;
> @@ -382,6 +392,7 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 
> projid)
>   EXT4_I(inode)->i_projid = kprojid;
>   inode->i_ctime = current_time(inode);
>  out_dirty:
> + up_read(_I(inode)->xattr_sem);
>