Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On Tue, Mar 16, 2021 at 08:05:31AM +0800, Qu Wenruo wrote: > > In that case one file with the list of supported values is a better > > option. The main point is to have full RW support, until then it's > > interesting only for developers and they know what to expect. > > > > Indeed only full RW support makes sense. > > BTW, any comment on the file name? If no problem I would just use > "supported_sectorsize" in next update. > > Although I hope the sysfs interface can be merged separately early, so > that I can add the proper support in btrfs-progs. Yeah, exporting the information via sysfs is the easy stuff so you can postpone it as you need.
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On Tue, Mar 16, 2021 at 08:10:13AM +0800, Anand Jain wrote: > > > On 16/03/2021 08:05, Qu Wenruo wrote: > > > > > > On 2021/3/16 上午2:44, David Sterba wrote: > >> On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote: > >>> > >>> > >>> On 2021/3/15 下午7:59, Anand Jain wrote: > On 10/03/2021 17:08, Qu Wenruo wrote: > > Add extra sysfs interface features/supported_ro_sectorsize and > > features/supported_rw_sectorsize to indicate subpage support. > > > > Currently for supported_rw_sectorsize all architectures only have > > their > > PAGE_SIZE listed. > > > > While for supported_ro_sectorsize, for systems with 64K page size, 4K > > sectorsize is also supported. > > > > This new sysfs interface would help mkfs.btrfs to do more accurate > > warning. > > > > Signed-off-by: Qu Wenruo > > --- > > Changes looks good. Nit below... > And maybe it is a good idea to wait for other comments before reroll. > > > fs/btrfs/sysfs.c | 34 ++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > > index 6eb1c50fa98c..3ef419899472 100644 > > --- a/fs/btrfs/sysfs.c > > +++ b/fs/btrfs/sysfs.c > > @@ -360,11 +360,45 @@ static ssize_t > > supported_rescue_options_show(struct kobject *kobj, > > BTRFS_ATTR(static_feature, supported_rescue_options, > > supported_rescue_options_show); > > +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, > > + struct kobj_attribute *a, > > + char *buf) > > +{ > > + ssize_t ret = 0; > > + int i = 0; > > Drop variable i, as ret can be used instead of 'i'. > > > + > > + /* For 64K page size, 4K sector size is supported */ > > + if (PAGE_SIZE == SZ_64K) { > > + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); > > + i++; > > + } > > + /* Other than above subpage, only support PAGE_SIZE as sectorsize > > yet */ > > + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", > > > + (i ? " " : ""), PAGE_SIZE); > ^ret > > > + return ret; > > +} > > +BTRFS_ATTR(static_feature, supported_ro_sectorsize, > > + supported_ro_sectorsize_show); > > + > > +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, > > + struct kobj_attribute *a, > > + char *buf) > > +{ > > + ssize_t ret = 0; > > + > > + /* Only PAGE_SIZE as sectorsize is supported */ > > + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); > > + return ret; > > +} > > +BTRFS_ATTR(static_feature, supported_rw_sectorsize, > > + supported_rw_sectorsize_show); > > Why not merge supported_ro_sectorsize and supported_rw_sectorsize > and show both in two lines... > For example: > cat supported_sectorsizes > ro: 4096 65536 > rw: 65536 > >>> > >>> If merged, btrfs-progs needs to do line number check before doing string > >>> matching. > >> > >> The sysfs files should do one value per file. > >> > >>> Although I doubt the usefulness for supported_ro_sectorsize, as the > >>> window for RO support without RW support should not be that large. > >>> (Current RW passes most generic test cases, and the remaining failures > >>> are very limited) > >>> > >>> Thus I can merged them into supported_sectorsize, and only report > >>> sectorsize we can do RW as supported. > >> > >> In that case one file with the list of supported values is a better > >> option. The main point is to have full RW support, until then it's > >> interesting only for developers and they know what to expect. > >> > > > > Indeed only full RW support makes sense. > > > Makes sense to me. > > > BTW, any comment on the file name? If no problem I would just use > > "supported_sectorsize" in next update. > > supported_sectorsizes (plural) is better IMO. Yeah pluar is consistent with what we have now, eg. supported_checksums
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On 16/03/2021 08:05, Qu Wenruo wrote: On 2021/3/16 上午2:44, David Sterba wrote: On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote: On 2021/3/15 下午7:59, Anand Jain wrote: On 10/03/2021 17:08, Qu Wenruo wrote: Add extra sysfs interface features/supported_ro_sectorsize and features/supported_rw_sectorsize to indicate subpage support. Currently for supported_rw_sectorsize all architectures only have their PAGE_SIZE listed. While for supported_ro_sectorsize, for systems with 64K page size, 4K sectorsize is also supported. This new sysfs interface would help mkfs.btrfs to do more accurate warning. Signed-off-by: Qu Wenruo --- Changes looks good. Nit below... And maybe it is a good idea to wait for other comments before reroll. fs/btrfs/sysfs.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 6eb1c50fa98c..3ef419899472 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj, BTRFS_ATTR(static_feature, supported_rescue_options, supported_rescue_options_show); +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + int i = 0; Drop variable i, as ret can be used instead of 'i'. + + /* For 64K page size, 4K sector size is supported */ + if (PAGE_SIZE == SZ_64K) { + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); + i++; + } + /* Other than above subpage, only support PAGE_SIZE as sectorsize yet */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", + (i ? " " : ""), PAGE_SIZE); ^ret + return ret; +} +BTRFS_ATTR(static_feature, supported_ro_sectorsize, + supported_ro_sectorsize_show); + +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + + /* Only PAGE_SIZE as sectorsize is supported */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_rw_sectorsize, + supported_rw_sectorsize_show); Why not merge supported_ro_sectorsize and supported_rw_sectorsize and show both in two lines... For example: cat supported_sectorsizes ro: 4096 65536 rw: 65536 If merged, btrfs-progs needs to do line number check before doing string matching. The sysfs files should do one value per file. Although I doubt the usefulness for supported_ro_sectorsize, as the window for RO support without RW support should not be that large. (Current RW passes most generic test cases, and the remaining failures are very limited) Thus I can merged them into supported_sectorsize, and only report sectorsize we can do RW as supported. In that case one file with the list of supported values is a better option. The main point is to have full RW support, until then it's interesting only for developers and they know what to expect. Indeed only full RW support makes sense. Makes sense to me. BTW, any comment on the file name? If no problem I would just use "supported_sectorsize" in next update. supported_sectorsizes (plural) is better IMO. Thanks, Anand Although I hope the sysfs interface can be merged separately early, so that I can add the proper support in btrfs-progs. Thanks, Qu
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On 2021/3/16 上午2:44, David Sterba wrote: On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote: On 2021/3/15 下午7:59, Anand Jain wrote: On 10/03/2021 17:08, Qu Wenruo wrote: Add extra sysfs interface features/supported_ro_sectorsize and features/supported_rw_sectorsize to indicate subpage support. Currently for supported_rw_sectorsize all architectures only have their PAGE_SIZE listed. While for supported_ro_sectorsize, for systems with 64K page size, 4K sectorsize is also supported. This new sysfs interface would help mkfs.btrfs to do more accurate warning. Signed-off-by: Qu Wenruo --- Changes looks good. Nit below... And maybe it is a good idea to wait for other comments before reroll. fs/btrfs/sysfs.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 6eb1c50fa98c..3ef419899472 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj, BTRFS_ATTR(static_feature, supported_rescue_options, supported_rescue_options_show); +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + int i = 0; Drop variable i, as ret can be used instead of 'i'. + + /* For 64K page size, 4K sector size is supported */ + if (PAGE_SIZE == SZ_64K) { + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); + i++; + } + /* Other than above subpage, only support PAGE_SIZE as sectorsize yet */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", + (i ? " " : ""), PAGE_SIZE); ^ret + return ret; +} +BTRFS_ATTR(static_feature, supported_ro_sectorsize, + supported_ro_sectorsize_show); + +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + + /* Only PAGE_SIZE as sectorsize is supported */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_rw_sectorsize, + supported_rw_sectorsize_show); Why not merge supported_ro_sectorsize and supported_rw_sectorsize and show both in two lines... For example: cat supported_sectorsizes ro: 4096 65536 rw: 65536 If merged, btrfs-progs needs to do line number check before doing string matching. The sysfs files should do one value per file. Although I doubt the usefulness for supported_ro_sectorsize, as the window for RO support without RW support should not be that large. (Current RW passes most generic test cases, and the remaining failures are very limited) Thus I can merged them into supported_sectorsize, and only report sectorsize we can do RW as supported. In that case one file with the list of supported values is a better option. The main point is to have full RW support, until then it's interesting only for developers and they know what to expect. Indeed only full RW support makes sense. BTW, any comment on the file name? If no problem I would just use "supported_sectorsize" in next update. Although I hope the sysfs interface can be merged separately early, so that I can add the proper support in btrfs-progs. Thanks, Qu
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote: > > > On 2021/3/15 下午7:59, Anand Jain wrote: > > On 10/03/2021 17:08, Qu Wenruo wrote: > >> Add extra sysfs interface features/supported_ro_sectorsize and > >> features/supported_rw_sectorsize to indicate subpage support. > >> > >> Currently for supported_rw_sectorsize all architectures only have their > >> PAGE_SIZE listed. > >> > >> While for supported_ro_sectorsize, for systems with 64K page size, 4K > >> sectorsize is also supported. > >> > >> This new sysfs interface would help mkfs.btrfs to do more accurate > >> warning. > >> > >> Signed-off-by: Qu Wenruo > >> --- > > > > Changes looks good. Nit below... > > And maybe it is a good idea to wait for other comments before reroll. > > > >> fs/btrfs/sysfs.c | 34 ++ > >> 1 file changed, 34 insertions(+) > >> > >> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > >> index 6eb1c50fa98c..3ef419899472 100644 > >> --- a/fs/btrfs/sysfs.c > >> +++ b/fs/btrfs/sysfs.c > >> @@ -360,11 +360,45 @@ static ssize_t > >> supported_rescue_options_show(struct kobject *kobj, > >> BTRFS_ATTR(static_feature, supported_rescue_options, > >> supported_rescue_options_show); > >> +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, > >> + struct kobj_attribute *a, > >> + char *buf) > >> +{ > >> + ssize_t ret = 0; > >> + int i = 0; > > > > Drop variable i, as ret can be used instead of 'i'. > > > >> + > >> + /* For 64K page size, 4K sector size is supported */ > >> + if (PAGE_SIZE == SZ_64K) { > >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); > >> + i++; > >> + } > >> + /* Other than above subpage, only support PAGE_SIZE as sectorsize > >> yet */ > >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", > > > >> + (i ? " " : ""), PAGE_SIZE); > > ^ret > > > >> + return ret; > >> +} > >> +BTRFS_ATTR(static_feature, supported_ro_sectorsize, > >> + supported_ro_sectorsize_show); > >> + > >> +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, > >> + struct kobj_attribute *a, > >> + char *buf) > >> +{ > >> + ssize_t ret = 0; > >> + > >> + /* Only PAGE_SIZE as sectorsize is supported */ > >> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); > >> + return ret; > >> +} > >> +BTRFS_ATTR(static_feature, supported_rw_sectorsize, > >> + supported_rw_sectorsize_show); > > > > Why not merge supported_ro_sectorsize and supported_rw_sectorsize > > and show both in two lines... > > For example: > > cat supported_sectorsizes > > ro: 4096 65536 > > rw: 65536 > > If merged, btrfs-progs needs to do line number check before doing string > matching. The sysfs files should do one value per file. > Although I doubt the usefulness for supported_ro_sectorsize, as the > window for RO support without RW support should not be that large. > (Current RW passes most generic test cases, and the remaining failures > are very limited) > > Thus I can merged them into supported_sectorsize, and only report > sectorsize we can do RW as supported. In that case one file with the list of supported values is a better option. The main point is to have full RW support, until then it's interesting only for developers and they know what to expect.
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On 2021/3/15 下午7:59, Anand Jain wrote: On 10/03/2021 17:08, Qu Wenruo wrote: Add extra sysfs interface features/supported_ro_sectorsize and features/supported_rw_sectorsize to indicate subpage support. Currently for supported_rw_sectorsize all architectures only have their PAGE_SIZE listed. While for supported_ro_sectorsize, for systems with 64K page size, 4K sectorsize is also supported. This new sysfs interface would help mkfs.btrfs to do more accurate warning. Signed-off-by: Qu Wenruo --- Changes looks good. Nit below... And maybe it is a good idea to wait for other comments before reroll. fs/btrfs/sysfs.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 6eb1c50fa98c..3ef419899472 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj, BTRFS_ATTR(static_feature, supported_rescue_options, supported_rescue_options_show); +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + int i = 0; Drop variable i, as ret can be used instead of 'i'. + + /* For 64K page size, 4K sector size is supported */ + if (PAGE_SIZE == SZ_64K) { + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); + i++; + } + /* Other than above subpage, only support PAGE_SIZE as sectorsize yet */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", + (i ? " " : ""), PAGE_SIZE); ^ret + return ret; +} +BTRFS_ATTR(static_feature, supported_ro_sectorsize, + supported_ro_sectorsize_show); + +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + + /* Only PAGE_SIZE as sectorsize is supported */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_rw_sectorsize, + supported_rw_sectorsize_show); Why not merge supported_ro_sectorsize and supported_rw_sectorsize and show both in two lines... For example: cat supported_sectorsizes ro: 4096 65536 rw: 65536 If merged, btrfs-progs needs to do line number check before doing string matching. Although I doubt the usefulness for supported_ro_sectorsize, as the window for RO support without RW support should not be that large. (Current RW passes most generic test cases, and the remaining failures are very limited) Thus I can merged them into supported_sectorsize, and only report sectorsize we can do RW as supported. Thanks, Qu static struct attribute *btrfs_supported_static_feature_attrs[] = { BTRFS_ATTR_PTR(static_feature, rmdir_subvol), BTRFS_ATTR_PTR(static_feature, supported_checksums), BTRFS_ATTR_PTR(static_feature, send_stream_version), BTRFS_ATTR_PTR(static_feature, supported_rescue_options), + BTRFS_ATTR_PTR(static_feature, supported_ro_sectorsize), + BTRFS_ATTR_PTR(static_feature, supported_rw_sectorsize), NULL };
Re: [PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
On 10/03/2021 17:08, Qu Wenruo wrote: Add extra sysfs interface features/supported_ro_sectorsize and features/supported_rw_sectorsize to indicate subpage support. Currently for supported_rw_sectorsize all architectures only have their PAGE_SIZE listed. While for supported_ro_sectorsize, for systems with 64K page size, 4K sectorsize is also supported. This new sysfs interface would help mkfs.btrfs to do more accurate warning. Signed-off-by: Qu Wenruo --- Changes looks good. Nit below... And maybe it is a good idea to wait for other comments before reroll. fs/btrfs/sysfs.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 6eb1c50fa98c..3ef419899472 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj, BTRFS_ATTR(static_feature, supported_rescue_options, supported_rescue_options_show); +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + int i = 0; Drop variable i, as ret can be used instead of 'i'. + + /* For 64K page size, 4K sector size is supported */ + if (PAGE_SIZE == SZ_64K) { + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); + i++; + } + /* Other than above subpage, only support PAGE_SIZE as sectorsize yet */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", +(i ? " " : ""), PAGE_SIZE); ^ret + return ret; +} +BTRFS_ATTR(static_feature, supported_ro_sectorsize, + supported_ro_sectorsize_show); + +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + + /* Only PAGE_SIZE as sectorsize is supported */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_rw_sectorsize, + supported_rw_sectorsize_show); Why not merge supported_ro_sectorsize and supported_rw_sectorsize and show both in two lines... For example: cat supported_sectorsizes ro: 4096 65536 rw: 65536 static struct attribute *btrfs_supported_static_feature_attrs[] = { BTRFS_ATTR_PTR(static_feature, rmdir_subvol), BTRFS_ATTR_PTR(static_feature, supported_checksums), BTRFS_ATTR_PTR(static_feature, send_stream_version), BTRFS_ATTR_PTR(static_feature, supported_rescue_options), + BTRFS_ATTR_PTR(static_feature, supported_ro_sectorsize), + BTRFS_ATTR_PTR(static_feature, supported_rw_sectorsize), NULL };
[PATCH v2 01/15] btrfs: add sysfs interface for supported sectorsize
Add extra sysfs interface features/supported_ro_sectorsize and features/supported_rw_sectorsize to indicate subpage support. Currently for supported_rw_sectorsize all architectures only have their PAGE_SIZE listed. While for supported_ro_sectorsize, for systems with 64K page size, 4K sectorsize is also supported. This new sysfs interface would help mkfs.btrfs to do more accurate warning. Signed-off-by: Qu Wenruo --- fs/btrfs/sysfs.c | 34 ++ 1 file changed, 34 insertions(+) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 6eb1c50fa98c..3ef419899472 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -360,11 +360,45 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj, BTRFS_ATTR(static_feature, supported_rescue_options, supported_rescue_options_show); +static ssize_t supported_ro_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + int i = 0; + + /* For 64K page size, 4K sector size is supported */ + if (PAGE_SIZE == SZ_64K) { + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u", SZ_4K); + i++; + } + /* Other than above subpage, only support PAGE_SIZE as sectorsize yet */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%lu\n", +(i ? " " : ""), PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_ro_sectorsize, + supported_ro_sectorsize_show); + +static ssize_t supported_rw_sectorsize_show(struct kobject *kobj, + struct kobj_attribute *a, + char *buf) +{ + ssize_t ret = 0; + + /* Only PAGE_SIZE as sectorsize is supported */ + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE); + return ret; +} +BTRFS_ATTR(static_feature, supported_rw_sectorsize, + supported_rw_sectorsize_show); static struct attribute *btrfs_supported_static_feature_attrs[] = { BTRFS_ATTR_PTR(static_feature, rmdir_subvol), BTRFS_ATTR_PTR(static_feature, supported_checksums), BTRFS_ATTR_PTR(static_feature, send_stream_version), BTRFS_ATTR_PTR(static_feature, supported_rescue_options), + BTRFS_ATTR_PTR(static_feature, supported_ro_sectorsize), + BTRFS_ATTR_PTR(static_feature, supported_rw_sectorsize), NULL }; -- 2.30.1