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);
>   

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

2017-05-31 Thread Tahsin Erdogan
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.

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);
rc = ext4_mark_iloc_dirty(handle, inode, );
if (!err)
err = rc;
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index caddc176a612..1d6fcbb01517 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -701,6 +701,60 @@ static void ext4_xattr_update_super_block(handle_t *handle,
}
 }
 
+int ext4_xattr_inode_count(struct inode *inode)
+{
+   struct ext4_iloc iloc = { .bh = NULL };
+   struct buffer_head *bh = NULL;
+   struct