On 16/03/2021 08:05, Qu Wenruo wrote:

On 2021/3/16 上午2:44, David Sterba wrote:

quoted

On Mon, Mar 15, 2021 at 08:39:31PM +0800, Qu Wenruo wrote:

quoted

On 2021/3/15 下午7:59, Anand Jain wrote:

quoted

On 10/03/2021 17:08, Qu Wenruo wrote:

quoted

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 <redacted>
---

Changes looks good. Nit below...
And maybe it is a good idea to wait for other comments before reroll.

quoted

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

quoted

+
+    /* 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",

quoted

+             (i ? " " : ""), PAGE_SIZE);

                            ^ret

quoted

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

quoted

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