linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled
@ 2020-10-15 11:32 Roman Anufriev
  2020-10-15 11:32 ` [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode Roman Anufriev
  2020-10-15 12:24 ` [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Jan Kara
  0 siblings, 2 replies; 7+ messages in thread
From: Roman Anufriev @ 2020-10-15 11:32 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, dmtrmonakhov, dotdot

Right now, there are several places, where we check whether quota
is enabled or journalled with quite long and non-self-descriptive
condition statements.

This patch wraps these statements into helpers (with naming along
with existing ones like sb_has_quota_loaded) for better readability
and easier usage.

Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
---
 fs/ext4/ext4.h      | 15 +++++++++++++++
 fs/ext4/ext4_jbd2.h |  9 +++------
 fs/ext4/super.c     |  5 +----
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 250e905..217ae7b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3251,6 +3251,21 @@ static inline void ext4_unlock_group(struct super_block *sb,
 	spin_unlock(ext4_group_lock_ptr(sb, group));
 }
 
+#ifdef CONFIG_QUOTA
+static inline bool ext4_any_quota_enabled(struct super_block *sb)
+{
+	return (test_opt(sb, QUOTA) || ext4_has_feature_quota(sb));
+}
+
+static inline bool ext4_is_quota_journalled(struct super_block *sb)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+	return (ext4_has_feature_quota(sb) ||
+		sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]);
+}
+#endif
+
 /*
  * Block validity checking
  */
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 00dc668..4a4eb3f 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -86,17 +86,14 @@
 #ifdef CONFIG_QUOTA
 /* Amount of blocks needed for quota update - we know that the structure was
  * allocated so we need to update only data block */
-#define EXT4_QUOTA_TRANS_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
-		ext4_has_feature_quota(sb)) ? 1 : 0)
+#define EXT4_QUOTA_TRANS_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ? 1 : 0)
 /* Amount of blocks needed for quota insert/delete - we do some block writes
  * but inode, sb and group updates are done only once */
-#define EXT4_QUOTA_INIT_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
-		ext4_has_feature_quota(sb)) ?\
+#define EXT4_QUOTA_INIT_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ?\
 		(DQUOT_INIT_ALLOC*(EXT4_SINGLEDATA_TRANS_BLOCKS(sb)-3)\
 		 +3+DQUOT_INIT_REWRITE) : 0)
 
-#define EXT4_QUOTA_DEL_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
-		ext4_has_feature_quota(sb)) ?\
+#define EXT4_QUOTA_DEL_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ?\
 		(DQUOT_DEL_ALLOC*(EXT4_SINGLEDATA_TRANS_BLOCKS(sb)-3)\
 		 +3+DQUOT_DEL_REWRITE) : 0)
 #else
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9d01318..a988cf3 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6158,11 +6158,8 @@ static int ext4_release_dquot(struct dquot *dquot)
 static int ext4_mark_dquot_dirty(struct dquot *dquot)
 {
 	struct super_block *sb = dquot->dq_sb;
-	struct ext4_sb_info *sbi = EXT4_SB(sb);
 
-	/* Are we journaling quotas? */
-	if (ext4_has_feature_quota(sb) ||
-	    sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
+	if (ext4_is_quota_journalled(sb)) {
 		dquot_mark_dquot_dirty(dquot);
 		return ext4_write_dquot(dquot);
 	} else {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode
  2020-10-15 11:32 [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Roman Anufriev
@ 2020-10-15 11:32 ` Roman Anufriev
  2020-10-15 13:15   ` Jan Kara
  2020-10-15 12:24 ` [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Jan Kara
  1 sibling, 1 reply; 7+ messages in thread
From: Roman Anufriev @ 2020-10-15 11:32 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, dmtrmonakhov, dotdot

Right now, it is hard to understand what quota journalling type is enabled:
you need to be quite familiar with kernel code and trace it or really
understand what different combinations of fs flags/mount options lead to.

This patch exports via sysfs attr /sys/fs/ext4/<disk>/quota_mode current
quota jounalling mode, making it easier to check at a glance/in autotests.
The semantics is similar to ext4 data journalling modes:

* journalled - quota accounting and journaling are enabled
* writeback  - quota accounting is enabled, but journalling is disabled
* none       - quota accounting is disabled
* disabled   - kernel compiled without CONFIG_QUOTA feature

Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
---
 fs/ext4/sysfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index bfabb79..a46487f 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -36,6 +36,7 @@ typedef enum {
 	attr_pointer_string,
 	attr_pointer_atomic,
 	attr_journal_task,
+	attr_quota_mode,
 } attr_id_t;
 
 typedef enum {
@@ -140,6 +141,23 @@ static ssize_t journal_task_show(struct ext4_sb_info *sbi, char *buf)
 			task_pid_vnr(sbi->s_journal->j_task));
 }
 
+static ssize_t quota_mode_show(struct ext4_sb_info *sbi, char *buf)
+{
+#ifdef CONFIG_QUOTA
+	struct super_block *sb = sbi->s_buddy_cache->i_sb;
+
+	if (!ext4_any_quota_enabled(sb))
+		return snprintf(buf, PAGE_SIZE, "none\n");
+
+	if (ext4_is_quota_journalled(sb))
+		return snprintf(buf, PAGE_SIZE, "journalled\n");
+	else
+		return snprintf(buf, PAGE_SIZE, "writeback\n");
+#else
+	return snprintf(buf, PAGE_SIZE, "disabled\n");
+#endif
+}
+
 #define EXT4_ATTR(_name,_mode,_id)					\
 static struct ext4_attr ext4_attr_##_name = {				\
 	.attr = {.name = __stringify(_name), .mode = _mode },		\
@@ -248,6 +266,7 @@ EXT4_ATTR(last_error_time, 0444, last_error_time);
 EXT4_ATTR(journal_task, 0444, journal_task);
 EXT4_RW_ATTR_SBI_UI(mb_prefetch, s_mb_prefetch);
 EXT4_RW_ATTR_SBI_UI(mb_prefetch_limit, s_mb_prefetch_limit);
+EXT4_ATTR_FUNC(quota_mode, 0444);
 
 static unsigned int old_bump_val = 128;
 EXT4_ATTR_PTR(max_writeback_mb_bump, 0444, pointer_ui, &old_bump_val);
@@ -296,6 +315,7 @@ static struct attribute *ext4_attrs[] = {
 #endif
 	ATTR_LIST(mb_prefetch),
 	ATTR_LIST(mb_prefetch_limit),
+	ATTR_LIST(quota_mode),
 	NULL,
 };
 ATTRIBUTE_GROUPS(ext4);
@@ -425,6 +445,8 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
 		return print_tstamp(buf, sbi->s_es, s_last_error_time);
 	case attr_journal_task:
 		return journal_task_show(sbi, buf);
+	case attr_quota_mode:
+		return quota_mode_show(sbi, buf);
 	}
 
 	return 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled
  2020-10-15 11:32 [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Roman Anufriev
  2020-10-15 11:32 ` [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode Roman Anufriev
@ 2020-10-15 12:24 ` Jan Kara
  1 sibling, 0 replies; 7+ messages in thread
From: Jan Kara @ 2020-10-15 12:24 UTC (permalink / raw)
  To: Roman Anufriev; +Cc: linux-ext4, tytso, jack, dmtrmonakhov

On Thu 15-10-20 14:32:51, Roman Anufriev wrote:
> Right now, there are several places, where we check whether quota
> is enabled or journalled with quite long and non-self-descriptive
> condition statements.
> 
> This patch wraps these statements into helpers (with naming along
> with existing ones like sb_has_quota_loaded) for better readability
> and easier usage.
> 
> Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
> Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/ext4.h      | 15 +++++++++++++++
>  fs/ext4/ext4_jbd2.h |  9 +++------
>  fs/ext4/super.c     |  5 +----
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 250e905..217ae7b 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3251,6 +3251,21 @@ static inline void ext4_unlock_group(struct super_block *sb,
>  	spin_unlock(ext4_group_lock_ptr(sb, group));
>  }
>  
> +#ifdef CONFIG_QUOTA
> +static inline bool ext4_any_quota_enabled(struct super_block *sb)
> +{
> +	return (test_opt(sb, QUOTA) || ext4_has_feature_quota(sb));
> +}
> +
> +static inline bool ext4_is_quota_journalled(struct super_block *sb)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	return (ext4_has_feature_quota(sb) ||
> +		sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]);
> +}
> +#endif
> +
>  /*
>   * Block validity checking
>   */
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 00dc668..4a4eb3f 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -86,17 +86,14 @@
>  #ifdef CONFIG_QUOTA
>  /* Amount of blocks needed for quota update - we know that the structure was
>   * allocated so we need to update only data block */
> -#define EXT4_QUOTA_TRANS_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
> -		ext4_has_feature_quota(sb)) ? 1 : 0)
> +#define EXT4_QUOTA_TRANS_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ? 1 : 0)
>  /* Amount of blocks needed for quota insert/delete - we do some block writes
>   * but inode, sb and group updates are done only once */
> -#define EXT4_QUOTA_INIT_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
> -		ext4_has_feature_quota(sb)) ?\
> +#define EXT4_QUOTA_INIT_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ?\
>  		(DQUOT_INIT_ALLOC*(EXT4_SINGLEDATA_TRANS_BLOCKS(sb)-3)\
>  		 +3+DQUOT_INIT_REWRITE) : 0)
>  
> -#define EXT4_QUOTA_DEL_BLOCKS(sb) ((test_opt(sb, QUOTA) ||\
> -		ext4_has_feature_quota(sb)) ?\
> +#define EXT4_QUOTA_DEL_BLOCKS(sb) ((ext4_any_quota_enabled(sb)) ?\
>  		(DQUOT_DEL_ALLOC*(EXT4_SINGLEDATA_TRANS_BLOCKS(sb)-3)\
>  		 +3+DQUOT_DEL_REWRITE) : 0)
>  #else
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 9d01318..a988cf3 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6158,11 +6158,8 @@ static int ext4_release_dquot(struct dquot *dquot)
>  static int ext4_mark_dquot_dirty(struct dquot *dquot)
>  {
>  	struct super_block *sb = dquot->dq_sb;
> -	struct ext4_sb_info *sbi = EXT4_SB(sb);
>  
> -	/* Are we journaling quotas? */
> -	if (ext4_has_feature_quota(sb) ||
> -	    sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
> +	if (ext4_is_quota_journalled(sb)) {
>  		dquot_mark_dquot_dirty(dquot);
>  		return ext4_write_dquot(dquot);
>  	} else {
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode
  2020-10-15 11:32 ` [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode Roman Anufriev
@ 2020-10-15 13:15   ` Jan Kara
  2020-10-17 18:26     ` Roman Anufriev
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2020-10-15 13:15 UTC (permalink / raw)
  To: Roman Anufriev; +Cc: linux-ext4, tytso, jack, dmtrmonakhov

On Thu 15-10-20 14:32:52, Roman Anufriev wrote:
> Right now, it is hard to understand what quota journalling type is enabled:
> you need to be quite familiar with kernel code and trace it or really
> understand what different combinations of fs flags/mount options lead to.
> 
> This patch exports via sysfs attr /sys/fs/ext4/<disk>/quota_mode current
> quota jounalling mode, making it easier to check at a glance/in autotests.
> The semantics is similar to ext4 data journalling modes:
> 
> * journalled - quota accounting and journaling are enabled
> * writeback  - quota accounting is enabled, but journalling is disabled
> * none       - quota accounting is disabled
> * disabled   - kernel compiled without CONFIG_QUOTA feature
> 
> Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
> Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>

Hum, I'm not sure about this. The state of quota can be found out with
"quotaon -p <mntpoint>" (or corresponding quotactl if you need this from
C). The only thing you won't learn is journalled / writeback mode and
generally you should not care about this although I agree that for fs
crash testing purposes you may care. But is that big enough usecase for a
new sysfs file when all the information is already available for userspace
just not in a convenient form?

BTW, I've now realized ext4_any_quota_enabled() has actually misleading
name in the sysfs file reports wrong information. It is rather
ext4_any_quota_may_be_enabled() since presence of QUOTA mount option only
says that quotaon(8) will enable quotas if it is run, not that quota
accounting is enabled. sb_any_quota_loaded() tells you if accounting is
actually enabled or not (however this can change anytime so that's why we
use more relaxed checks for the purpose of journal credit estimates).

								Honza

> ---
>  fs/ext4/sysfs.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index bfabb79..a46487f 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -36,6 +36,7 @@ typedef enum {
>  	attr_pointer_string,
>  	attr_pointer_atomic,
>  	attr_journal_task,
> +	attr_quota_mode,
>  } attr_id_t;
>  
>  typedef enum {
> @@ -140,6 +141,23 @@ static ssize_t journal_task_show(struct ext4_sb_info *sbi, char *buf)
>  			task_pid_vnr(sbi->s_journal->j_task));
>  }
>  
> +static ssize_t quota_mode_show(struct ext4_sb_info *sbi, char *buf)
> +{
> +#ifdef CONFIG_QUOTA
> +	struct super_block *sb = sbi->s_buddy_cache->i_sb;
> +
> +	if (!ext4_any_quota_enabled(sb))
> +		return snprintf(buf, PAGE_SIZE, "none\n");
> +
> +	if (ext4_is_quota_journalled(sb))
> +		return snprintf(buf, PAGE_SIZE, "journalled\n");
> +	else
> +		return snprintf(buf, PAGE_SIZE, "writeback\n");
> +#else
> +	return snprintf(buf, PAGE_SIZE, "disabled\n");
> +#endif
> +}
> +
>  #define EXT4_ATTR(_name,_mode,_id)					\
>  static struct ext4_attr ext4_attr_##_name = {				\
>  	.attr = {.name = __stringify(_name), .mode = _mode },		\
> @@ -248,6 +266,7 @@ EXT4_ATTR(last_error_time, 0444, last_error_time);
>  EXT4_ATTR(journal_task, 0444, journal_task);
>  EXT4_RW_ATTR_SBI_UI(mb_prefetch, s_mb_prefetch);
>  EXT4_RW_ATTR_SBI_UI(mb_prefetch_limit, s_mb_prefetch_limit);
> +EXT4_ATTR_FUNC(quota_mode, 0444);
>  
>  static unsigned int old_bump_val = 128;
>  EXT4_ATTR_PTR(max_writeback_mb_bump, 0444, pointer_ui, &old_bump_val);
> @@ -296,6 +315,7 @@ static struct attribute *ext4_attrs[] = {
>  #endif
>  	ATTR_LIST(mb_prefetch),
>  	ATTR_LIST(mb_prefetch_limit),
> +	ATTR_LIST(quota_mode),
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(ext4);
> @@ -425,6 +445,8 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
>  		return print_tstamp(buf, sbi->s_es, s_last_error_time);
>  	case attr_journal_task:
>  		return journal_task_show(sbi, buf);
> +	case attr_quota_mode:
> +		return quota_mode_show(sbi, buf);
>  	}
>  
>  	return 0;
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode
  2020-10-15 13:15   ` Jan Kara
@ 2020-10-17 18:26     ` Roman Anufriev
  2020-10-19  8:19       ` Jan Kara
  0 siblings, 1 reply; 7+ messages in thread
From: Roman Anufriev @ 2020-10-17 18:26 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, tytso, dmtrmonakhov

Hi, sorry for the delay.

On Thu, 15 Oct 2020, Jan Kara wrote:

> On Thu 15-10-20 14:32:52, Roman Anufriev wrote:
>> Right now, it is hard to understand what quota journalling type is enabled:
>> you need to be quite familiar with kernel code and trace it or really
>> understand what different combinations of fs flags/mount options lead to.
>>
>> This patch exports via sysfs attr /sys/fs/ext4/<disk>/quota_mode current
>> quota jounalling mode, making it easier to check at a glance/in autotests.
>> The semantics is similar to ext4 data journalling modes:
>>
>> * journalled - quota accounting and journaling are enabled
>> * writeback  - quota accounting is enabled, but journalling is disabled
>> * none       - quota accounting is disabled
>> * disabled   - kernel compiled without CONFIG_QUOTA feature
>>
>> Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
>> Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
>
> Hum, I'm not sure about this. The state of quota can be found out with
> "quotaon -p <mntpoint>" (or corresponding quotactl if you need this from
> C). The only thing you won't learn is journalled / writeback mode and
> generally you should not care about this although I agree that for fs
> crash testing purposes you may care. But is that big enough usecase for a
> new sysfs file when all the information is already available for userspace
> just not in a convenient form?

Rationale behind this patch was mainly the addition of an easy way to 
check whether quota journalled or not as this is quite wanted feature in 
out production environment. TBH, I was not sure about sysfs file too, but 
it seemed to me like the most natural place to put it. Maybe if sysfs is 
an overkill - just add printing to dmesg on mount? At least, you'll be 
able to check what quota type you can enable right after mounting.

> BTW, I've now realized ext4_any_quota_enabled() has actually misleading
> name in the sysfs file reports wrong information. It is rather
> ext4_any_quota_may_be_enabled() since presence of QUOTA mount option only
> says that quotaon(8) will enable quotas if it is run, not that quota
> accounting is enabled. sb_any_quota_loaded() tells you if accounting is
> actually enabled or not (however this can change anytime so that's why we
> use more relaxed checks for the purpose of journal credit estimates).

My bad! Totally forgot about the case when 'quota' mount option is present 
but quota accounting is not enabled, as our helper-tool around 
'quotactl()' do remounting+enabling in one go.

I'll rename the function to smth like 'ext4_quota_capable()' in v2. And if 
printing to dmesg is OK, I'll probably still use this function to print on 
mount what the quota type will be after accounting is enabled.

 								Roman

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode
  2020-10-17 18:26     ` Roman Anufriev
@ 2020-10-19  8:19       ` Jan Kara
  2020-10-19  9:36         ` Roman Anufriev
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Kara @ 2020-10-19  8:19 UTC (permalink / raw)
  To: Roman Anufriev; +Cc: Jan Kara, linux-ext4, tytso, dmtrmonakhov

On Sat 17-10-20 21:26:37, Roman Anufriev wrote:
> Hi, sorry for the delay.
> 
> On Thu, 15 Oct 2020, Jan Kara wrote:
> 
> > On Thu 15-10-20 14:32:52, Roman Anufriev wrote:
> > > Right now, it is hard to understand what quota journalling type is enabled:
> > > you need to be quite familiar with kernel code and trace it or really
> > > understand what different combinations of fs flags/mount options lead to.
> > > 
> > > This patch exports via sysfs attr /sys/fs/ext4/<disk>/quota_mode current
> > > quota jounalling mode, making it easier to check at a glance/in autotests.
> > > The semantics is similar to ext4 data journalling modes:
> > > 
> > > * journalled - quota accounting and journaling are enabled
> > > * writeback  - quota accounting is enabled, but journalling is disabled
> > > * none       - quota accounting is disabled
> > > * disabled   - kernel compiled without CONFIG_QUOTA feature
> > > 
> > > Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
> > > Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
> > 
> > Hum, I'm not sure about this. The state of quota can be found out with
> > "quotaon -p <mntpoint>" (or corresponding quotactl if you need this from
> > C). The only thing you won't learn is journalled / writeback mode and
> > generally you should not care about this although I agree that for fs
> > crash testing purposes you may care. But is that big enough usecase for a
> > new sysfs file when all the information is already available for userspace
> > just not in a convenient form?
> 
> Rationale behind this patch was mainly the addition of an easy way to check
> whether quota journalled or not as this is quite wanted feature in out
> production environment. TBH, I was not sure about sysfs file too, but it
> seemed to me like the most natural place to put it. Maybe if sysfs is an
> overkill - just add printing to dmesg on mount? At least, you'll be able to
> check what quota type you can enable right after mounting.
> 
> > BTW, I've now realized ext4_any_quota_enabled() has actually misleading
> > name in the sysfs file reports wrong information. It is rather
> > ext4_any_quota_may_be_enabled() since presence of QUOTA mount option only
> > says that quotaon(8) will enable quotas if it is run, not that quota
> > accounting is enabled. sb_any_quota_loaded() tells you if accounting is
> > actually enabled or not (however this can change anytime so that's why we
> > use more relaxed checks for the purpose of journal credit estimates).
> 
> My bad! Totally forgot about the case when 'quota' mount option is present
> but quota accounting is not enabled, as our helper-tool around 'quotactl()'
> do remounting+enabling in one go.
> 
> I'll rename the function to smth like 'ext4_quota_capable()' in v2. And if
> printing to dmesg is OK, I'll probably still use this function to print on
> mount what the quota type will be after accounting is enabled.

Yeah, if a message in dmesg is fine for your purposes, I'd rather go with
that than with a sysfs file.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode
  2020-10-19  8:19       ` Jan Kara
@ 2020-10-19  9:36         ` Roman Anufriev
  0 siblings, 0 replies; 7+ messages in thread
From: Roman Anufriev @ 2020-10-19  9:36 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, tytso, dmtrmonakhov

On Mon, 19 Oct 2020, Jan Kara wrote:

> On Sat 17-10-20 21:26:37, Roman Anufriev wrote:
>> Hi, sorry for the delay.
>>
>> On Thu, 15 Oct 2020, Jan Kara wrote:
>>
>>> On Thu 15-10-20 14:32:52, Roman Anufriev wrote:
>>>> Right now, it is hard to understand what quota journalling type is enabled:
>>>> you need to be quite familiar with kernel code and trace it or really
>>>> understand what different combinations of fs flags/mount options lead to.
>>>>
>>>> This patch exports via sysfs attr /sys/fs/ext4/<disk>/quota_mode current
>>>> quota jounalling mode, making it easier to check at a glance/in autotests.
>>>> The semantics is similar to ext4 data journalling modes:
>>>>
>>>> * journalled - quota accounting and journaling are enabled
>>>> * writeback  - quota accounting is enabled, but journalling is disabled
>>>> * none       - quota accounting is disabled
>>>> * disabled   - kernel compiled without CONFIG_QUOTA feature
>>>>
>>>> Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
>>>> Reviewed-by: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
>>>
>>> Hum, I'm not sure about this. The state of quota can be found out with
>>> "quotaon -p <mntpoint>" (or corresponding quotactl if you need this from
>>> C). The only thing you won't learn is journalled / writeback mode and
>>> generally you should not care about this although I agree that for fs
>>> crash testing purposes you may care. But is that big enough usecase for a
>>> new sysfs file when all the information is already available for userspace
>>> just not in a convenient form?
>>
>> Rationale behind this patch was mainly the addition of an easy way to check
>> whether quota journalled or not as this is quite wanted feature in out
>> production environment. TBH, I was not sure about sysfs file too, but it
>> seemed to me like the most natural place to put it. Maybe if sysfs is an
>> overkill - just add printing to dmesg on mount? At least, you'll be able to
>> check what quota type you can enable right after mounting.
>>
>>> BTW, I've now realized ext4_any_quota_enabled() has actually misleading
>>> name in the sysfs file reports wrong information. It is rather
>>> ext4_any_quota_may_be_enabled() since presence of QUOTA mount option only
>>> says that quotaon(8) will enable quotas if it is run, not that quota
>>> accounting is enabled. sb_any_quota_loaded() tells you if accounting is
>>> actually enabled or not (however this can change anytime so that's why we
>>> use more relaxed checks for the purpose of journal credit estimates).
>>
>> My bad! Totally forgot about the case when 'quota' mount option is present
>> but quota accounting is not enabled, as our helper-tool around 'quotactl()'
>> do remounting+enabling in one go.
>>
>> I'll rename the function to smth like 'ext4_quota_capable()' in v2. And if
>> printing to dmesg is OK, I'll probably still use this function to print on
>> mount what the quota type will be after accounting is enabled.
>
> Yeah, if a message in dmesg is fine for your purposes, I'd rather go with
> that than with a sysfs file.

I think it'll be enough. I've implemented this in v3 (skip the v2, please 
- made a syntax mistake there): 
https://lore.kernel.org/linux-ext4/1603099162-25028-1-git-send-email-dotdot@yandex-team.ru/

 								Roman

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-10-19  9:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 11:32 [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Roman Anufriev
2020-10-15 11:32 ` [PATCH 2/2] ext4: export quota journalling mode via sysfs attr quota_mode Roman Anufriev
2020-10-15 13:15   ` Jan Kara
2020-10-17 18:26     ` Roman Anufriev
2020-10-19  8:19       ` Jan Kara
2020-10-19  9:36         ` Roman Anufriev
2020-10-15 12:24 ` [PATCH 1/2] ext4: add helpers for checking whether quota is enabled/journalled Jan Kara

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