| Message ID | 1720028821-3094-1-git-send-email-zhanglikernel@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs-prog: scrub: print message when scrubbing a read-only filesystem without r option | expand |
在 2024/7/4 03:17, Li Zhang 写道: > issue:666 It's better to move the issue tag before your SOB line. And with something like "Issue: #829" so that github can detect it. Check commit ab2260355afb ("btrfs-progs: subvol list: fix accidental trimming of subvolume name") for a proper example. > > [enhancement] > When scrubbing a filesystem mounted read-only and without r > option, it aborts and there is no message associated with it. > So we need to print an error message when scrubbing a read-only > filesystem to tell the user what is going on here. > > [implementation] > Move the error message from the main thread to each scrub thread, > previously the message was printed after all scrub threads > finished and without background mode. > > [test] > Mount dev in read-only mode, then scrub it without r option > > $ sudo mount /dev/vdb -o ro /mnt/ > $ sudo btrfs scrub start /mnt/ > scrub started on /mnt/, fsid f7c26803-a68d-4a96-91c4-a629b57b7f64 (pid=2892) > Starting scrub on devid 1 > Starting scrub on devid 2 > Starting scrub on devid 3 > scrub on devid 1 failed ret=-1 errno=30 (Read-only file system) > scrub on devid 2 failed ret=-1 errno=30 (Read-only file system) > scrub on devid 3 failed ret=-1 errno=30 (Read-only file system) > > Signed-off-by: Li Zhang <zhanglikernel@gmail.com> > --- > cmds/scrub.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/cmds/scrub.c b/cmds/scrub.c > index d54e11f..e0400dd 100644 > --- a/cmds/scrub.c > +++ b/cmds/scrub.c > @@ -957,7 +957,10 @@ static void *scrub_one_dev(void *ctx) > warning("setting ioprio failed: %m (ignored)"); > > ret = ioctl(sp->fd, BTRFS_IOC_SCRUB, &sp->scrub_args); > - pr_verbose(LOG_DEFAULT, "scrub ioctl devid:%llu ret:%d errno:%d\n",sp->scrub_args.devid, ret, errno); > + if (ret){ > + pr_verbose(LOG_DEFAULT, "scrub on devid %llu failed ret=%d errno=%d (%m)\n", > + sp->scrub_args.devid, ret, errno); > + } Doing direct output inside a thread can lead to race of the output. And we do not know if the scrub ioctl would return error early or run for a long long time, that's the dilemma. I'm wondering if it's possible to do a blkid/mountinfo based probe. Then it would avoid the possible output race and error out earilier. Thanks, Qu > gettimeofday(&tv, NULL); > sp->ret = ret; > sp->stats.duration = tv.tv_sec - sp->stats.t_start; > @@ -1596,13 +1599,6 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv, > ++err; > break; > default: > - if (do_print) { > - errno = sp[i].ioctl_errno; > - error( > - "scrubbing %s failed for device id %lld: ret=%d, errno=%d (%m)", > - path, devid, sp[i].ret, > - sp[i].ioctl_errno); > - } > ++err; > continue; > }
On Thu, Jul 04, 2024 at 08:56:47AM +0930, Qu Wenruo wrote: > > --- a/cmds/scrub.c > > +++ b/cmds/scrub.c > > @@ -957,7 +957,10 @@ static void *scrub_one_dev(void *ctx) > > warning("setting ioprio failed: %m (ignored)"); > > > > ret = ioctl(sp->fd, BTRFS_IOC_SCRUB, &sp->scrub_args); > > - pr_verbose(LOG_DEFAULT, "scrub ioctl devid:%llu ret:%d errno:%d\n",sp->scrub_args.devid, ret, errno); > > + if (ret){ > > + pr_verbose(LOG_DEFAULT, "scrub on devid %llu failed ret=%d errno=%d (%m)\n", > > + sp->scrub_args.devid, ret, errno); > > + } > > Doing direct output inside a thread can lead to race of the output. We'd have to communicate the ioctl start errors back to the main thread, or rely on internal locking of printf, that it can race is possible but as long as the string is printed in one go we'll get only lines reordered. > And we do not know if the scrub ioctl would return error early or run > for a long long time, that's the dilemma. > > I'm wondering if it's possible to do a blkid/mountinfo based probe. > Then it would avoid the possible output race and error out earilier. We can detet read/write status of the block devices. I'm not sure about the mount info, what function can we use for that?
在 2024/7/4 09:43, David Sterba 写道: > On Thu, Jul 04, 2024 at 08:56:47AM +0930, Qu Wenruo wrote: >>> --- a/cmds/scrub.c >>> +++ b/cmds/scrub.c >>> @@ -957,7 +957,10 @@ static void *scrub_one_dev(void *ctx) >>> warning("setting ioprio failed: %m (ignored)"); >>> >>> ret = ioctl(sp->fd, BTRFS_IOC_SCRUB, &sp->scrub_args); >>> - pr_verbose(LOG_DEFAULT, "scrub ioctl devid:%llu ret:%d errno:%d\n",sp->scrub_args.devid, ret, errno); >>> + if (ret){ >>> + pr_verbose(LOG_DEFAULT, "scrub on devid %llu failed ret=%d errno=%d (%m)\n", >>> + sp->scrub_args.devid, ret, errno); >>> + } >> >> Doing direct output inside a thread can lead to race of the output. > > We'd have to communicate the ioctl start errors back to the main thread, > or rely on internal locking of printf, that it can race is possible but > as long as the string is printed in one go we'll get only lines > reordered. > >> And we do not know if the scrub ioctl would return error early or run >> for a long long time, that's the dilemma. >> >> I'm wondering if it's possible to do a blkid/mountinfo based probe. >> Then it would avoid the possible output race and error out earilier. > > We can detet read/write status of the block devices. I'm not sure about > the mount info, what function can we use for that? > E.g, setmntent() and getmntent() used inside check_mounted_where()? Since mntent structure provides mnt_opts, it should not be that hard to find if the destination mount point is mounted ro? Thanks, Qu
On Thu, Jul 04, 2024 at 09:49:52AM +0930, Qu Wenruo wrote: > E.g, setmntent() and getmntent() used inside check_mounted_where()? > > Since mntent structure provides mnt_opts, it should not be that hard to > find if the destination mount point is mounted ro? Oh right, it's in getmntent. I was looking there but expected a bit mask, the options are strings and the convenience helper for checking if they're set is hasmntopt().
Yes, I think it's a good idea to get the mntent before scrub, but
my first thought was to write the errno to the status file
/var/lib/btrfs/scrub.status.{fsid},
since today we deal with read-only files System, there may be other
errno that need
to be processed in the future. It seems more accurate to write errno
into status, and
we can also get the scrub status in the scrub status command. But I
wonder if this
will cause backward compatibility issues.
Thanks,
Li
David Sterba <dsterba@suse.cz> 于2024年7月4日周四 08:22写道：
>
> On Thu, Jul 04, 2024 at 09:49:52AM +0930, Qu Wenruo wrote:
> > E.g, setmntent() and getmntent() used inside check_mounted_where()?
> >
> > Since mntent structure provides mnt_opts, it should not be that hard to
> > find if the destination mount point is mounted ro?
>
> Oh right, it's in getmntent. I was looking there but expected a bit
> mask, the options are strings and the convenience helper for checking if
> they're set is hasmntopt().
diff --git a/cmds/scrub.c b/cmds/scrub.c index d54e11f..e0400dd 100644 --- a/cmds/scrub.c +++ b/cmds/scrub.c @@ -957,7 +957,10 @@ static void *scrub_one_dev(void *ctx) warning("setting ioprio failed: %m (ignored)"); ret = ioctl(sp->fd, BTRFS_IOC_SCRUB, &sp->scrub_args); - pr_verbose(LOG_DEFAULT, "scrub ioctl devid:%llu ret:%d errno:%d\n",sp->scrub_args.devid, ret, errno); + if (ret){ + pr_verbose(LOG_DEFAULT, "scrub on devid %llu failed ret=%d errno=%d (%m)\n", + sp->scrub_args.devid, ret, errno); + } gettimeofday(&tv, NULL); sp->ret = ret; sp->stats.duration = tv.tv_sec - sp->stats.t_start; @@ -1596,13 +1599,6 @@ static int scrub_start(const struct cmd_struct *cmd, int argc, char **argv, ++err; break; default: - if (do_print) { - errno = sp[i].ioctl_errno; - error( - "scrubbing %s failed for device id %lld: ret=%d, errno=%d (%m)", - path, devid, sp[i].ret, - sp[i].ioctl_errno); - } ++err; continue; }
issue:666 [enhancement] When scrubbing a filesystem mounted read-only and without r option, it aborts and there is no message associated with it. So we need to print an error message when scrubbing a read-only filesystem to tell the user what is going on here. [implementation] Move the error message from the main thread to each scrub thread, previously the message was printed after all scrub threads finished and without background mode. [test] Mount dev in read-only mode, then scrub it without r option $ sudo mount /dev/vdb -o ro /mnt/ $ sudo btrfs scrub start /mnt/ scrub started on /mnt/, fsid f7c26803-a68d-4a96-91c4-a629b57b7f64 (pid=2892) Starting scrub on devid 1 Starting scrub on devid 2 Starting scrub on devid 3 scrub on devid 1 failed ret=-1 errno=30 (Read-only file system) scrub on devid 2 failed ret=-1 errno=30 (Read-only file system) scrub on devid 3 failed ret=-1 errno=30 (Read-only file system) Signed-off-by: Li Zhang <zhanglikernel@gmail.com> --- cmds/scrub.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)