* [PATCH] ext4: fix quota charging for shared xattr blocks @ 2017-05-23 3:01 Tahsin Erdogan 2017-05-23 8:10 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Tahsin Erdogan @ 2017-05-23 3:01 UTC (permalink / raw) To: Theodore Ts'o, Andreas Dilger Cc: linux-ext4, linux-kernel, Tahsin Erdogan ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr block when new references are made. However if dquot_initialize() hasn't been called on an inode, request for charging is effectively ignored because ext4_inode_info->i_dquot is not initialized yet. Add dquot_initialize() call to ext4_xattr_set_handle(). Signed-off-by: Tahsin Erdogan <tahsin@google.com> --- fs/ext4/xattr.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 8fb7ce14e6eb..e94575448550 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -1166,6 +1166,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, return -EINVAL; if (strlen(name) > 255) return -ERANGE; + + error = dquot_initialize(inode); + if (error) + return error; + ext4_write_lock_xattr(inode, &no_expand); error = ext4_reserve_inode_write(handle, inode, &is.iloc); -- 2.13.0.219.gdb65acc882-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: fix quota charging for shared xattr blocks 2017-05-23 3:01 [PATCH] ext4: fix quota charging for shared xattr blocks Tahsin Erdogan @ 2017-05-23 8:10 ` Jan Kara 2017-05-23 23:03 ` [PATCH v2] " Tahsin Erdogan 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2017-05-23 8:10 UTC (permalink / raw) To: Tahsin Erdogan Cc: Theodore Ts'o, Andreas Dilger, linux-ext4, linux-kernel On Mon 22-05-17 20:01:48, Tahsin Erdogan wrote: > ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr > block when new references are made. However if dquot_initialize() hasn't > been called on an inode, request for charging is effectively ignored > because ext4_inode_info->i_dquot is not initialized yet. > > Add dquot_initialize() call to ext4_xattr_set_handle(). Thanks for finding the bug! However this is a wrong place where to insert dquot_initialize(). Generally we try really hard to do dquot_initalize() calls outside of a transaction (as they may have pretty big requirements on the amount of transaction credits - and for your patch to be correct you'd have to update credit estimates for each possible transaction involved). So you should rather place these calls into ext4_xattr_set(), ext4_set_acl(), ext4_set_context() (only in the case when transaction needs to be started before starting it). You can then add a warning in ext4_xattr_set_handle() checking whether quota is indeed initialized as expected. Honza > > Signed-off-by: Tahsin Erdogan <tahsin@google.com> > --- > fs/ext4/xattr.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 8fb7ce14e6eb..e94575448550 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -1166,6 +1166,11 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > return -EINVAL; > if (strlen(name) > 255) > return -ERANGE; > + > + error = dquot_initialize(inode); > + if (error) > + return error; > + > ext4_write_lock_xattr(inode, &no_expand); > > error = ext4_reserve_inode_write(handle, inode, &is.iloc); > -- > 2.13.0.219.gdb65acc882-goog > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ext4: fix quota charging for shared xattr blocks 2017-05-23 8:10 ` Jan Kara @ 2017-05-23 23:03 ` Tahsin Erdogan 2017-05-24 9:06 ` Jan Kara 0 siblings, 1 reply; 6+ messages in thread From: Tahsin Erdogan @ 2017-05-23 23:03 UTC (permalink / raw) To: Theodore Ts'o, Andreas Dilger, Jan Kara Cc: linux-ext4, linux-kernel, Tahsin Erdogan ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr block when new references are made. However if dquot_initialize() hasn't been called on an inode, request for charging is effectively ignored because ext4_inode_info->i_dquot is not initialized yet. Add dquot_initialize() to call paths that lead to ext4_xattr_block_set(). Signed-off-by: Tahsin Erdogan <tahsin@google.com> --- v2: - moved dquot_initialize() to outside the main transaction - added dquot_initialize_needed() function that is used by ext4_xattr_release_block() to warn about call paths that don't call dquot_initialize() fs/ext4/acl.c | 4 ++++ fs/ext4/super.c | 3 +++ fs/ext4/xattr.c | 8 ++++++++ fs/quota/dquot.c | 16 ++++++++++++++++ include/linux/quotaops.h | 6 ++++++ 5 files changed, 37 insertions(+) diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index fd389935ecd1..3ec0e46de95f 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -4,6 +4,7 @@ * Copyright (C) 2001-2003 Andreas Gruenbacher, <agruen@suse.de> */ +#include <linux/quotaops.h> #include "ext4_jbd2.h" #include "ext4.h" #include "xattr.h" @@ -232,6 +233,9 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type) handle_t *handle; int error, retries = 0; + error = dquot_initialize(inode); + if (error) + return error; retry: handle = ext4_journal_start(inode, EXT4_HT_XATTR, ext4_jbd2_credits_xattr(inode)); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 0b177da9ea82..e2e0eb7ac9a0 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1179,6 +1179,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, return res; } + res = dquot_initialize(inode); + if (res) + return res; retry: handle = ext4_journal_start(inode, EXT4_HT_MISC, ext4_jbd2_credits_xattr(inode)); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 8fb7ce14e6eb..5d3c2536641c 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -888,6 +888,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, else { u32 ref; + WARN_ON_ONCE(dquot_initialize_needed(inode)); + /* The old block is released after updating the inode. */ error = dquot_alloc_block(inode, @@ -954,6 +956,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, /* We need to allocate a new block */ ext4_fsblk_t goal, block; + WARN_ON_ONCE(dquot_initialize_needed(inode)); + goal = ext4_group_first_block_no(sb, EXT4_I(inode)->i_block_group); @@ -1166,6 +1170,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, return -EINVAL; if (strlen(name) > 255) return -ERANGE; + ext4_write_lock_xattr(inode, &no_expand); error = ext4_reserve_inode_write(handle, inode, &is.iloc); @@ -1267,6 +1272,9 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name, int error, retries = 0; int credits = ext4_jbd2_credits_xattr(inode); + error = dquot_initialize(inode); + if (error) + return error; retry: handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits); if (IS_ERR(handle)) { diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index ebf80c7739e1..48813aeaab80 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1512,6 +1512,22 @@ int dquot_initialize(struct inode *inode) } EXPORT_SYMBOL(dquot_initialize); +bool dquot_initialize_needed(struct inode *inode) +{ + struct dquot **dquots; + int i; + + if (!dquot_active(inode)) + return false; + + dquots = i_dquot(inode); + for (i = 0; i < MAXQUOTAS; i++) + if (!dquots[i] && sb_has_quota_active(inode->i_sb, i)) + return true; + return false; +} +EXPORT_SYMBOL(dquot_initialize_needed); + /* * Release all quotas referenced by inode. * diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h index 9c6f768b7d32..dda22f45fc1b 100644 --- a/include/linux/quotaops.h +++ b/include/linux/quotaops.h @@ -44,6 +44,7 @@ void inode_sub_rsv_space(struct inode *inode, qsize_t number); void inode_reclaim_rsv_space(struct inode *inode, qsize_t number); int dquot_initialize(struct inode *inode); +bool dquot_initialize_needed(struct inode *inode); void dquot_drop(struct inode *inode); struct dquot *dqget(struct super_block *sb, struct kqid qid); static inline struct dquot *dqgrab(struct dquot *dquot) @@ -207,6 +208,11 @@ static inline int dquot_initialize(struct inode *inode) return 0; } +static inline bool dquot_initialize_needed(struct inode *inode) +{ + return false; +} + static inline void dquot_drop(struct inode *inode) { } -- 2.13.0.219.gdb65acc882-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ext4: fix quota charging for shared xattr blocks 2017-05-23 23:03 ` [PATCH v2] " Tahsin Erdogan @ 2017-05-24 9:06 ` Jan Kara 2017-05-24 11:35 ` [PATCH v3] " Tahsin Erdogan 0 siblings, 1 reply; 6+ messages in thread From: Jan Kara @ 2017-05-24 9:06 UTC (permalink / raw) To: Tahsin Erdogan Cc: Theodore Ts'o, Andreas Dilger, Jan Kara, linux-ext4, linux-kernel On Tue 23-05-17 16:03:51, Tahsin Erdogan wrote: > ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr > block when new references are made. However if dquot_initialize() hasn't > been called on an inode, request for charging is effectively ignored > because ext4_inode_info->i_dquot is not initialized yet. > > Add dquot_initialize() to call paths that lead to ext4_xattr_block_set(). > > Signed-off-by: Tahsin Erdogan <tahsin@google.com> Looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > v2: > - moved dquot_initialize() to outside the main transaction > - added dquot_initialize_needed() function that is used by > ext4_xattr_release_block() to warn about call paths that don't > call dquot_initialize() > > fs/ext4/acl.c | 4 ++++ > fs/ext4/super.c | 3 +++ > fs/ext4/xattr.c | 8 ++++++++ > fs/quota/dquot.c | 16 ++++++++++++++++ > include/linux/quotaops.h | 6 ++++++ > 5 files changed, 37 insertions(+) > > diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c > index fd389935ecd1..3ec0e46de95f 100644 > --- a/fs/ext4/acl.c > +++ b/fs/ext4/acl.c > @@ -4,6 +4,7 @@ > * Copyright (C) 2001-2003 Andreas Gruenbacher, <agruen@suse.de> > */ > > +#include <linux/quotaops.h> > #include "ext4_jbd2.h" > #include "ext4.h" > #include "xattr.h" > @@ -232,6 +233,9 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type) > handle_t *handle; > int error, retries = 0; > > + error = dquot_initialize(inode); > + if (error) > + return error; > retry: > handle = ext4_journal_start(inode, EXT4_HT_XATTR, > ext4_jbd2_credits_xattr(inode)); > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 0b177da9ea82..e2e0eb7ac9a0 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -1179,6 +1179,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, > return res; > } > > + res = dquot_initialize(inode); > + if (res) > + return res; > retry: > handle = ext4_journal_start(inode, EXT4_HT_MISC, > ext4_jbd2_credits_xattr(inode)); > diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c > index 8fb7ce14e6eb..5d3c2536641c 100644 > --- a/fs/ext4/xattr.c > +++ b/fs/ext4/xattr.c > @@ -888,6 +888,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, > else { > u32 ref; > > + WARN_ON_ONCE(dquot_initialize_needed(inode)); > + > /* The old block is released after updating > the inode. */ > error = dquot_alloc_block(inode, > @@ -954,6 +956,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, > /* We need to allocate a new block */ > ext4_fsblk_t goal, block; > > + WARN_ON_ONCE(dquot_initialize_needed(inode)); > + > goal = ext4_group_first_block_no(sb, > EXT4_I(inode)->i_block_group); > > @@ -1166,6 +1170,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, > return -EINVAL; > if (strlen(name) > 255) > return -ERANGE; > + > ext4_write_lock_xattr(inode, &no_expand); > > error = ext4_reserve_inode_write(handle, inode, &is.iloc); > @@ -1267,6 +1272,9 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name, > int error, retries = 0; > int credits = ext4_jbd2_credits_xattr(inode); > > + error = dquot_initialize(inode); > + if (error) > + return error; > retry: > handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits); > if (IS_ERR(handle)) { > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index ebf80c7739e1..48813aeaab80 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -1512,6 +1512,22 @@ int dquot_initialize(struct inode *inode) > } > EXPORT_SYMBOL(dquot_initialize); > > +bool dquot_initialize_needed(struct inode *inode) > +{ > + struct dquot **dquots; > + int i; > + > + if (!dquot_active(inode)) > + return false; > + > + dquots = i_dquot(inode); > + for (i = 0; i < MAXQUOTAS; i++) > + if (!dquots[i] && sb_has_quota_active(inode->i_sb, i)) > + return true; > + return false; > +} > +EXPORT_SYMBOL(dquot_initialize_needed); > + > /* > * Release all quotas referenced by inode. > * > diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h > index 9c6f768b7d32..dda22f45fc1b 100644 > --- a/include/linux/quotaops.h > +++ b/include/linux/quotaops.h > @@ -44,6 +44,7 @@ void inode_sub_rsv_space(struct inode *inode, qsize_t number); > void inode_reclaim_rsv_space(struct inode *inode, qsize_t number); > > int dquot_initialize(struct inode *inode); > +bool dquot_initialize_needed(struct inode *inode); > void dquot_drop(struct inode *inode); > struct dquot *dqget(struct super_block *sb, struct kqid qid); > static inline struct dquot *dqgrab(struct dquot *dquot) > @@ -207,6 +208,11 @@ static inline int dquot_initialize(struct inode *inode) > return 0; > } > > +static inline bool dquot_initialize_needed(struct inode *inode) > +{ > + return false; > +} > + > static inline void dquot_drop(struct inode *inode) > { > } > -- > 2.13.0.219.gdb65acc882-goog > -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] ext4: fix quota charging for shared xattr blocks 2017-05-24 9:06 ` Jan Kara @ 2017-05-24 11:35 ` Tahsin Erdogan 2017-05-24 22:25 ` Theodore Ts'o 0 siblings, 1 reply; 6+ messages in thread From: Tahsin Erdogan @ 2017-05-24 11:35 UTC (permalink / raw) To: Theodore Ts'o, Andreas Dilger, Jan Kara Cc: linux-ext4, linux-kernel, Tahsin Erdogan ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr block when new references are made. However if dquot_initialize() hasn't been called on an inode, request for charging is effectively ignored because ext4_inode_info->i_dquot is not initialized yet. Add dquot_initialize() to call paths that lead to ext4_xattr_block_set(). Signed-off-by: Tahsin Erdogan <tahsin@google.com> Reviewed-by: Jan Kara <jack@suse.cz> --- v3: added Reviewed-by tag v2: - moved dquot_initialize() to outside the main transaction - added dquot_initialize_needed() function that is used by ext4_xattr_release_block() to warn about call paths that don't call dquot_initialize() fs/ext4/acl.c | 4 ++++ fs/ext4/super.c | 3 +++ fs/ext4/xattr.c | 8 ++++++++ fs/quota/dquot.c | 16 ++++++++++++++++ include/linux/quotaops.h | 6 ++++++ 5 files changed, 37 insertions(+) diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index fd389935ecd1..3ec0e46de95f 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -4,6 +4,7 @@ * Copyright (C) 2001-2003 Andreas Gruenbacher, <agruen@suse.de> */ +#include <linux/quotaops.h> #include "ext4_jbd2.h" #include "ext4.h" #include "xattr.h" @@ -232,6 +233,9 @@ ext4_set_acl(struct inode *inode, struct posix_acl *acl, int type) handle_t *handle; int error, retries = 0; + error = dquot_initialize(inode); + if (error) + return error; retry: handle = ext4_journal_start(inode, EXT4_HT_XATTR, ext4_jbd2_credits_xattr(inode)); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 0b177da9ea82..e2e0eb7ac9a0 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1179,6 +1179,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len, return res; } + res = dquot_initialize(inode); + if (res) + return res; retry: handle = ext4_journal_start(inode, EXT4_HT_MISC, ext4_jbd2_credits_xattr(inode)); diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c index 8fb7ce14e6eb..5d3c2536641c 100644 --- a/fs/ext4/xattr.c +++ b/fs/ext4/xattr.c @@ -888,6 +888,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, else { u32 ref; + WARN_ON_ONCE(dquot_initialize_needed(inode)); + /* The old block is released after updating the inode. */ error = dquot_alloc_block(inode, @@ -954,6 +956,8 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode, /* We need to allocate a new block */ ext4_fsblk_t goal, block; + WARN_ON_ONCE(dquot_initialize_needed(inode)); + goal = ext4_group_first_block_no(sb, EXT4_I(inode)->i_block_group); @@ -1166,6 +1170,7 @@ ext4_xattr_set_handle(handle_t *handle, struct inode *inode, int name_index, return -EINVAL; if (strlen(name) > 255) return -ERANGE; + ext4_write_lock_xattr(inode, &no_expand); error = ext4_reserve_inode_write(handle, inode, &is.iloc); @@ -1267,6 +1272,9 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name, int error, retries = 0; int credits = ext4_jbd2_credits_xattr(inode); + error = dquot_initialize(inode); + if (error) + return error; retry: handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits); if (IS_ERR(handle)) { diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index ebf80c7739e1..48813aeaab80 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -1512,6 +1512,22 @@ int dquot_initialize(struct inode *inode) } EXPORT_SYMBOL(dquot_initialize); +bool dquot_initialize_needed(struct inode *inode) +{ + struct dquot **dquots; + int i; + + if (!dquot_active(inode)) + return false; + + dquots = i_dquot(inode); + for (i = 0; i < MAXQUOTAS; i++) + if (!dquots[i] && sb_has_quota_active(inode->i_sb, i)) + return true; + return false; +} +EXPORT_SYMBOL(dquot_initialize_needed); + /* * Release all quotas referenced by inode. * diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h index 9c6f768b7d32..dda22f45fc1b 100644 --- a/include/linux/quotaops.h +++ b/include/linux/quotaops.h @@ -44,6 +44,7 @@ void inode_sub_rsv_space(struct inode *inode, qsize_t number); void inode_reclaim_rsv_space(struct inode *inode, qsize_t number); int dquot_initialize(struct inode *inode); +bool dquot_initialize_needed(struct inode *inode); void dquot_drop(struct inode *inode); struct dquot *dqget(struct super_block *sb, struct kqid qid); static inline struct dquot *dqgrab(struct dquot *dquot) @@ -207,6 +208,11 @@ static inline int dquot_initialize(struct inode *inode) return 0; } +static inline bool dquot_initialize_needed(struct inode *inode) +{ + return false; +} + static inline void dquot_drop(struct inode *inode) { } -- 2.13.0.219.gdb65acc882-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ext4: fix quota charging for shared xattr blocks 2017-05-24 11:35 ` [PATCH v3] " Tahsin Erdogan @ 2017-05-24 22:25 ` Theodore Ts'o 0 siblings, 0 replies; 6+ messages in thread From: Theodore Ts'o @ 2017-05-24 22:25 UTC (permalink / raw) To: Tahsin Erdogan; +Cc: Andreas Dilger, Jan Kara, linux-ext4, linux-kernel On Wed, May 24, 2017 at 04:35:19AM -0700, Tahsin Erdogan wrote: > ext4_xattr_block_set() calls dquot_alloc_block() to charge for an xattr > block when new references are made. However if dquot_initialize() hasn't > been called on an inode, request for charging is effectively ignored > because ext4_inode_info->i_dquot is not initialized yet. > > Add dquot_initialize() to call paths that lead to ext4_xattr_block_set(). > > Signed-off-by: Tahsin Erdogan <tahsin@google.com> > Reviewed-by: Jan Kara <jack@suse.cz> Applied, thanks. - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-24 22:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-23 3:01 [PATCH] ext4: fix quota charging for shared xattr blocks Tahsin Erdogan 2017-05-23 8:10 ` Jan Kara 2017-05-23 23:03 ` [PATCH v2] " Tahsin Erdogan 2017-05-24 9:06 ` Jan Kara 2017-05-24 11:35 ` [PATCH v3] " Tahsin Erdogan 2017-05-24 22:25 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).