| Message ID | b30a6712179ead9c8a5556d82a4ac386904c9b4d.1657265564.git.jstancek@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | tst_find_backing_dev: fix logic in matching mount point | expand |
On Fri, Jul 8, 2022 at 3:33 PM Jan Stancek <jstancek@redhat.com> wrote: > If backing dev is btrfs root device, then starting best_match_len > from 1 creates an issue, because root (/) is never matched. > Also we should check that entire mount point string is present in > path we are matching against. > > In case there's error also dump /proc/self/mountinfo before tst_brk. > > This fixes test with following partition layout (TMPDIR is on /): > # cat /proc/self/mountinfo | grep btrfs > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 > rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 > rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > Make sense! Reviewed-by: Li Wang <liwang@redhat.com>
Hello, Jan Stancek <jstancek@redhat.com> writes: > If backing dev is btrfs root device, then starting best_match_len > from 1 creates an issue, because root (/) is never matched. > Also we should check that entire mount point string is present in > path we are matching against. > > In case there's error also dump /proc/self/mountinfo before tst_brk. > > This fixes test with following partition layout (TMPDIR is on /): > # cat /proc/self/mountinfo | grep btrfs > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > --- > lib/tst_device.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/lib/tst_device.c b/lib/tst_device.c > index c34cbe6d1f56..414bf0eea816 100644 > --- a/lib/tst_device.c > +++ b/lib/tst_device.c > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) > return dev_bytes_written; > } > > -static int count_match_len(const char *first, const char *second) > +static int str_starts_with(const char *str, const char *prefix) > { > int len = 0; > > - while (*first && *first++ == *second++) > + while (*prefix) { > + if (!*str) > + return 0; > + if (*str++ != *prefix++) > + return 0; > len++; > + } I'm not sure this is better than the original. It's a seperate cleanup in any case. > > return len; > } > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) > char *pre = NULL; > char *next = NULL; > unsigned int dev_major, dev_minor, line_mjr, line_mnr; > - unsigned int len, best_match_len = 1; > + unsigned int len, best_match_len = 0; > char mnt_point[PATH_MAX]; > > if (stat(path, &buf) < 0) > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) > break; > } > > - len = count_match_len(path, mnt_point); > + len = str_starts_with(path, mnt_point); > if (len > best_match_len) { > strcpy(dev, pre); > best_match_len = len; > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) > > SAFE_FCLOSE(NULL, file); > > - if (!*dev) > + if (!*dev) { > + tst_system("cat /proc/self/mountinfo"); > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); > + } > > if (stat(dev, &buf) < 0) > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); > -- > 2.27.0 Makes sense. However I suspect this function can be replaced with the standard library method used in tst_stat_mount_dev. I didn't try this before because I'm not sure why it scans mountinfo instead of mounts.
On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > > Hello, > > Jan Stancek <jstancek@redhat.com> writes: > > > If backing dev is btrfs root device, then starting best_match_len > > from 1 creates an issue, because root (/) is never matched. > > Also we should check that entire mount point string is present in > > path we are matching against. > > > > In case there's error also dump /proc/self/mountinfo before tst_brk. > > > > This fixes test with following partition layout (TMPDIR is on /): > > # cat /proc/self/mountinfo | grep btrfs > > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root > > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > --- > > lib/tst_device.c | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/lib/tst_device.c b/lib/tst_device.c > > index c34cbe6d1f56..414bf0eea816 100644 > > --- a/lib/tst_device.c > > +++ b/lib/tst_device.c > > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) > > return dev_bytes_written; > > } > > > > -static int count_match_len(const char *first, const char *second) > > +static int str_starts_with(const char *str, const char *prefix) > > { > > int len = 0; > > > > - while (*first && *first++ == *second++) > > + while (*prefix) { > > + if (!*str) > > + return 0; > > + if (*str++ != *prefix++) > > + return 0; > > len++; > > + } > > I'm not sure this is better than the original. It's a seperate cleanup > in any case. The difference is that partial matches now returns 0. Previously it did not, it only counted number of matching characters. So for example ("/foobar" "/foo") appeared as better match than ("/foobar", "/") > > > > > return len; > > } > > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) > > char *pre = NULL; > > char *next = NULL; > > unsigned int dev_major, dev_minor, line_mjr, line_mnr; > > - unsigned int len, best_match_len = 1; > > + unsigned int len, best_match_len = 0; > > char mnt_point[PATH_MAX]; > > > > if (stat(path, &buf) < 0) > > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) > > break; > > } > > > > - len = count_match_len(path, mnt_point); > > + len = str_starts_with(path, mnt_point); > > if (len > best_match_len) { > > strcpy(dev, pre); > > best_match_len = len; > > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) > > > > SAFE_FCLOSE(NULL, file); > > > > - if (!*dev) > > + if (!*dev) { > > + tst_system("cat /proc/self/mountinfo"); > > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); > > + } > > > > if (stat(dev, &buf) < 0) > > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); > > -- > > 2.27.0 > > Makes sense. However I suspect this function can be replaced with the > standard library method used in tst_stat_mount_dev. I didn't try this > before because I'm not sure why it scans mountinfo instead of mounts. /proc/self/mounts doesn't contain major/minor numbers, which is primarily what tst_find_backing_dev is using. > > > -- > Thank you, > Richard. >
Hello, Jan Stancek <jstancek@redhat.com> writes: > On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: >> >> Hello, >> >> Jan Stancek <jstancek@redhat.com> writes: >> >> > If backing dev is btrfs root device, then starting best_match_len >> > from 1 creates an issue, because root (/) is never matched. >> > Also we should check that entire mount point string is present in >> > path we are matching against. >> > >> > In case there's error also dump /proc/self/mountinfo before tst_brk. >> > >> > This fixes test with following partition layout (TMPDIR is on /): >> > # cat /proc/self/mountinfo | grep btrfs >> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root >> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home >> > >> > Signed-off-by: Jan Stancek <jstancek@redhat.com> >> > --- >> > lib/tst_device.c | 17 ++++++++++++----- >> > 1 file changed, 12 insertions(+), 5 deletions(-) >> > >> > diff --git a/lib/tst_device.c b/lib/tst_device.c >> > index c34cbe6d1f56..414bf0eea816 100644 >> > --- a/lib/tst_device.c >> > +++ b/lib/tst_device.c >> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) >> > return dev_bytes_written; >> > } >> > >> > -static int count_match_len(const char *first, const char *second) >> > +static int str_starts_with(const char *str, const char *prefix) >> > { >> > int len = 0; >> > >> > - while (*first && *first++ == *second++) >> > + while (*prefix) { >> > + if (!*str) >> > + return 0; >> > + if (*str++ != *prefix++) >> > + return 0; >> > len++; >> > + } >> >> I'm not sure this is better than the original. It's a seperate cleanup >> in any case. > > The difference is that partial matches now returns 0. Previously it did not, > it only counted number of matching characters. So for example ("/foobar" "/foo") > appeared as better match than ("/foobar", "/") Ah, sorry. > >> >> > >> > return len; >> > } >> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) >> > char *pre = NULL; >> > char *next = NULL; >> > unsigned int dev_major, dev_minor, line_mjr, line_mnr; >> > - unsigned int len, best_match_len = 1; >> > + unsigned int len, best_match_len = 0; >> > char mnt_point[PATH_MAX]; >> > >> > if (stat(path, &buf) < 0) >> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) >> > break; >> > } >> > >> > - len = count_match_len(path, mnt_point); >> > + len = str_starts_with(path, mnt_point); >> > if (len > best_match_len) { >> > strcpy(dev, pre); >> > best_match_len = len; >> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) >> > >> > SAFE_FCLOSE(NULL, file); >> > >> > - if (!*dev) >> > + if (!*dev) { >> > + tst_system("cat /proc/self/mountinfo"); >> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); >> > + } >> > >> > if (stat(dev, &buf) < 0) >> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); >> > -- >> > 2.27.0 >> >> Makes sense. However I suspect this function can be replaced with the >> standard library method used in tst_stat_mount_dev. I didn't try this >> before because I'm not sure why it scans mountinfo instead of mounts. > > /proc/self/mounts doesn't contain major/minor numbers, which is primarily what > tst_find_backing_dev is using. Does anyone know a situation where checking the mount path doesn't work? It seems like we are just checking the device numbers first for historical reasons and not because it is necessary. I haven't seen any failure reports for io_control01 which only checks the path with tst_stat_mount_dev. However this test only runs if you have CG v2 enabled by default. > >> >> >> -- >> Thank you, >> Richard. >>
On Mon, Jul 11, 2022 at 3:21 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > > Hello, > > Jan Stancek <jstancek@redhat.com> writes: > > > On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: > >> > >> Hello, > >> > >> Jan Stancek <jstancek@redhat.com> writes: > >> > >> > If backing dev is btrfs root device, then starting best_match_len > >> > from 1 creates an issue, because root (/) is never matched. > >> > Also we should check that entire mount point string is present in > >> > path we are matching against. > >> > > >> > In case there's error also dump /proc/self/mountinfo before tst_brk. > >> > > >> > This fixes test with following partition layout (TMPDIR is on /): > >> > # cat /proc/self/mountinfo | grep btrfs > >> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root > >> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home > >> > > >> > Signed-off-by: Jan Stancek <jstancek@redhat.com> > >> > --- > >> > lib/tst_device.c | 17 ++++++++++++----- > >> > 1 file changed, 12 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/lib/tst_device.c b/lib/tst_device.c > >> > index c34cbe6d1f56..414bf0eea816 100644 > >> > --- a/lib/tst_device.c > >> > +++ b/lib/tst_device.c > >> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) > >> > return dev_bytes_written; > >> > } > >> > > >> > -static int count_match_len(const char *first, const char *second) > >> > +static int str_starts_with(const char *str, const char *prefix) > >> > { > >> > int len = 0; > >> > > >> > - while (*first && *first++ == *second++) > >> > + while (*prefix) { > >> > + if (!*str) > >> > + return 0; > >> > + if (*str++ != *prefix++) > >> > + return 0; > >> > len++; > >> > + } > >> > >> I'm not sure this is better than the original. It's a seperate cleanup > >> in any case. > > > > The difference is that partial matches now returns 0. Previously it did not, > > it only counted number of matching characters. So for example ("/foobar" "/foo") > > appeared as better match than ("/foobar", "/") > > Ah, sorry. > > > > >> > >> > > >> > return len; > >> > } > >> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) > >> > char *pre = NULL; > >> > char *next = NULL; > >> > unsigned int dev_major, dev_minor, line_mjr, line_mnr; > >> > - unsigned int len, best_match_len = 1; > >> > + unsigned int len, best_match_len = 0; > >> > char mnt_point[PATH_MAX]; > >> > > >> > if (stat(path, &buf) < 0) > >> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) > >> > break; > >> > } > >> > > >> > - len = count_match_len(path, mnt_point); > >> > + len = str_starts_with(path, mnt_point); > >> > if (len > best_match_len) { > >> > strcpy(dev, pre); > >> > best_match_len = len; > >> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) > >> > > >> > SAFE_FCLOSE(NULL, file); > >> > > >> > - if (!*dev) > >> > + if (!*dev) { > >> > + tst_system("cat /proc/self/mountinfo"); > >> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); > >> > + } > >> > > >> > if (stat(dev, &buf) < 0) > >> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); > >> > -- > >> > 2.27.0 > >> > >> Makes sense. However I suspect this function can be replaced with the > >> standard library method used in tst_stat_mount_dev. I didn't try this > >> before because I'm not sure why it scans mountinfo instead of mounts. > > > > /proc/self/mounts doesn't contain major/minor numbers, which is primarily what > > tst_find_backing_dev is using. > > Does anyone know a situation where checking the mount path doesn't work? You could mount multiple times to same mount path, which would make it harder to find the right one. > It seems like we are just checking the device numbers first for > historical reasons and not because it is necessary. Device numbers seem slightly less ambiguous. > > I haven't seen any failure reports for io_control01 which only checks > the path with tst_stat_mount_dev. However this test only runs if you > have CG v2 enabled by default. > > > > >> > >> > >> -- > >> Thank you, > >> Richard. > >> > > > -- > Thank you, > Richard. >
Hello, Jan Stancek <jstancek@redhat.com> writes: > On Mon, Jul 11, 2022 at 3:21 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: >> >> Hello, >> >> Jan Stancek <jstancek@redhat.com> writes: >> >> > On Mon, Jul 11, 2022 at 2:13 PM Richard Palethorpe <rpalethorpe@suse.de> wrote: >> >> >> >> Hello, >> >> >> >> Jan Stancek <jstancek@redhat.com> writes: >> >> >> >> > If backing dev is btrfs root device, then starting best_match_len >> >> > from 1 creates an issue, because root (/) is never matched. >> >> > Also we should check that entire mount point string is present in >> >> > path we are matching against. >> >> > >> >> > In case there's error also dump /proc/self/mountinfo before tst_brk. >> >> > >> >> > This fixes test with following partition layout (TMPDIR is on /): >> >> > # cat /proc/self/mountinfo | grep btrfs >> >> > 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root >> >> > 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home >> >> > >> >> > Signed-off-by: Jan Stancek <jstancek@redhat.com> >> >> > --- >> >> > lib/tst_device.c | 17 ++++++++++++----- >> >> > 1 file changed, 12 insertions(+), 5 deletions(-) >> >> > >> >> > diff --git a/lib/tst_device.c b/lib/tst_device.c >> >> > index c34cbe6d1f56..414bf0eea816 100644 >> >> > --- a/lib/tst_device.c >> >> > +++ b/lib/tst_device.c >> >> > @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) >> >> > return dev_bytes_written; >> >> > } >> >> > >> >> > -static int count_match_len(const char *first, const char *second) >> >> > +static int str_starts_with(const char *str, const char *prefix) >> >> > { >> >> > int len = 0; >> >> > >> >> > - while (*first && *first++ == *second++) >> >> > + while (*prefix) { >> >> > + if (!*str) >> >> > + return 0; >> >> > + if (*str++ != *prefix++) >> >> > + return 0; >> >> > len++; >> >> > + } >> >> >> >> I'm not sure this is better than the original. It's a seperate cleanup >> >> in any case. >> > >> > The difference is that partial matches now returns 0. Previously it did not, >> > it only counted number of matching characters. So for example ("/foobar" "/foo") >> > appeared as better match than ("/foobar", "/") >> >> Ah, sorry. >> >> > >> >> >> >> > >> >> > return len; >> >> > } >> >> > @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) >> >> > char *pre = NULL; >> >> > char *next = NULL; >> >> > unsigned int dev_major, dev_minor, line_mjr, line_mnr; >> >> > - unsigned int len, best_match_len = 1; >> >> > + unsigned int len, best_match_len = 0; >> >> > char mnt_point[PATH_MAX]; >> >> > >> >> > if (stat(path, &buf) < 0) >> >> > @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) >> >> > break; >> >> > } >> >> > >> >> > - len = count_match_len(path, mnt_point); >> >> > + len = str_starts_with(path, mnt_point); >> >> > if (len > best_match_len) { >> >> > strcpy(dev, pre); >> >> > best_match_len = len; >> >> > @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) >> >> > >> >> > SAFE_FCLOSE(NULL, file); >> >> > >> >> > - if (!*dev) >> >> > + if (!*dev) { >> >> > + tst_system("cat /proc/self/mountinfo"); >> >> > tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); >> >> > + } >> >> > >> >> > if (stat(dev, &buf) < 0) >> >> > tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev); >> >> > -- >> >> > 2.27.0 >> >> >> >> Makes sense. However I suspect this function can be replaced with the >> >> standard library method used in tst_stat_mount_dev. I didn't try this >> >> before because I'm not sure why it scans mountinfo instead of mounts. >> > >> > /proc/self/mounts doesn't contain major/minor numbers, which is primarily what >> > tst_find_backing_dev is using. >> >> Does anyone know a situation where checking the mount path doesn't work? > > You could mount multiple times to same mount path, which would make it harder > to find the right one. > >> It seems like we are just checking the device numbers first for >> historical reasons and not because it is necessary. > > Device numbers seem slightly less ambiguous. I'm still not entirely sure about this, but if you want to merge then please go ahead. Acked-by: Richard Palethorpe <rpalethorpe@suse.com> > >> >> I haven't seen any failure reports for io_control01 which only checks >> the path with tst_stat_mount_dev. However this test only runs if you >> have CG v2 enabled by default. >> >> > >> >> >> >> >> >> -- >> >> Thank you, >> >> Richard. >> >> >> >> >> -- >> Thank you, >> Richard. >>
diff --git a/lib/tst_device.c b/lib/tst_device.c index c34cbe6d1f56..414bf0eea816 100644 --- a/lib/tst_device.c +++ b/lib/tst_device.c @@ -506,12 +506,17 @@ unsigned long tst_dev_bytes_written(const char *dev) return dev_bytes_written; } -static int count_match_len(const char *first, const char *second) +static int str_starts_with(const char *str, const char *prefix) { int len = 0; - while (*first && *first++ == *second++) + while (*prefix) { + if (!*str) + return 0; + if (*str++ != *prefix++) + return 0; len++; + } return len; } @@ -524,7 +529,7 @@ void tst_find_backing_dev(const char *path, char *dev) char *pre = NULL; char *next = NULL; unsigned int dev_major, dev_minor, line_mjr, line_mnr; - unsigned int len, best_match_len = 1; + unsigned int len, best_match_len = 0; char mnt_point[PATH_MAX]; if (stat(path, &buf) < 0) @@ -550,7 +555,7 @@ void tst_find_backing_dev(const char *path, char *dev) break; } - len = count_match_len(path, mnt_point); + len = str_starts_with(path, mnt_point); if (len > best_match_len) { strcpy(dev, pre); best_match_len = len; @@ -559,8 +564,10 @@ void tst_find_backing_dev(const char *path, char *dev) SAFE_FCLOSE(NULL, file); - if (!*dev) + if (!*dev) { + tst_system("cat /proc/self/mountinfo"); tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); + } if (stat(dev, &buf) < 0) tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
If backing dev is btrfs root device, then starting best_match_len from 1 creates an issue, because root (/) is never matched. Also we should check that entire mount point string is present in path we are matching against. In case there's error also dump /proc/self/mountinfo before tst_brk. This fixes test with following partition layout (TMPDIR is on /): # cat /proc/self/mountinfo | grep btrfs 59 1 0:29 /root / rw,relatime shared:1 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=257,subvol=/root 93 59 0:29 /home /home rw,relatime shared:47 - btrfs /dev/dasda2 rw,seclabel,compress=zstd:1,ssd,space_cache=v2,subvolid=256,subvol=/home Signed-off-by: Jan Stancek <jstancek@redhat.com> --- lib/tst_device.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)