[PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[Goldwyn Rodrigues]

check_running_fs_exclop() can return 1 when exclop is changed to "none"
The ret is set by the return value of the select() operation. Checking
the exclusive op changes just the exclop variable while ret is still
set to 1.

Set ret = 0 if exclop is set to BTRFS_EXCL_NONE or BTRFS_EXCL_UNKNOWN.
Remove unnecessary continue statement at the end of the block.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 common/utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/utils.c b/common/utils.c
index 1627913a..3c562247 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -1771,7 +1771,8 @@ int check_running_fs_exclop(int fd, enum exclusive_operation start, bool enqueue
 			tv.tv_sec /= 2;
 			ret = select(sysfs_fd + 1, NULL, NULL, &fds, &tv);
 			exclop = get_fs_exclop(fd);
-			continue;
+			if (exclop <= 0)
+				ret = 0;
 		}
 	}
 out:
-- 
2.32.0


-- 
Goldwyn

Re: [PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[David Sterba]

On Mon, Jun 28, 2021 at 02:40:00PM -0500, Goldwyn Rodrigues wrote:
> check_running_fs_exclop() can return 1 when exclop is changed to "none"
> The ret is set by the return value of the select() operation. Checking
> the exclusive op changes just the exclop variable while ret is still
> set to 1.
> 
> Set ret = 0 if exclop is set to BTRFS_EXCL_NONE or BTRFS_EXCL_UNKNOWN.
> Remove unnecessary continue statement at the end of the block.

That's describing what the code does in words, but there must be some
user visible effects like failed command. Do you have some reproducer?

I've applied patch as it's a fix but would still like to update the
changelog.

Re: [PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[Goldwyn Rodrigues]

On 21:21 01/07, David Sterba wrote:
> On Mon, Jun 28, 2021 at 02:40:00PM -0500, Goldwyn Rodrigues wrote:
> > check_running_fs_exclop() can return 1 when exclop is changed to "none"
> > The ret is set by the return value of the select() operation. Checking
> > the exclusive op changes just the exclop variable while ret is still
> > set to 1.
> > 
> > Set ret = 0 if exclop is set to BTRFS_EXCL_NONE or BTRFS_EXCL_UNKNOWN.
> > Remove unnecessary continue statement at the end of the block.
> 
> That's describing what the code does in words, but there must be some
> user visible effects like failed command. Do you have some reproducer?
> 
> I've applied patch as it's a fix but would still like to update the
> changelog.

If check_running_fs_exclop() returns anything but zero, it would exit
immediately and the ioctl, for which the program is called, is not
executed, without any error notice. IOW, the command appears to have
executed, but does not. This was found when balance which typically
reports chunks relocated did not print anything on screen.

-- 
Goldwyn

Re: [PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[David Sterba]

On Mon, Apr 12, 2021 at 10:56:41AM -0500, Goldwyn Rodrigues wrote:
> On  6:50 10/04, Anand Jain wrote:
> > On 09/04/2021 23:56, Goldwyn Rodrigues wrote:
> > > check_running_fs_exclop() can return 1 when exclop is changed to "none"
> > > The ret is set by the return value of the select() operation. Checking
> > > the exclusive op changes just the exclop variable while ret is still
> > > set to 1.
> > > 
> > > Set ret exclusively if exclop is set to BTRFS_EXCL_NONE.
> > > ---
> > 
> > SOB missing.
> 
> Yes, missed that.
> 
> > 
> > >   common/utils.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/common/utils.c b/common/utils.c
> > > index 57e41432..2e5175c3 100644
> > > --- a/common/utils.c
> > > +++ b/common/utils.c
> > > @@ -2326,6 +2326,8 @@ int check_running_fs_exclop(int fd, enum exclusive_operation start, bool enqueue
> > >   			tv.tv_sec /= 2;
> > >   			ret = select(sysfs_fd + 1, NULL, NULL, &fds, &tv);
> > >   			exclop = get_fs_exclop(fd);
> > > +			if (exclop == BTRFS_EXCL_NONE)
> > > +				ret = 0;
> > >   			continue;
> > >   		}
> > >   	}
> > > 
> > 
> > 
> > This is bit inconsistent from what is done a few lines above:
> > 
> >         exclop = get_fs_exclop(fd);
> >         if (exclop <= 0) {
> >                 ret = 0;
> >                 goto out;
> >         }
> > 
> > We return 0 for both BTRFS_EXCLOP_NONE || BTRFS_EXCLOP_UNKNOWN.
> > 
> 
> I am not sure why we are translating the sysfs file value to enums. The
> only status we are concerned about is with "none", anything besides that
> is considered to be busy, for code flow purposes. For error display, we
> can print whatever the sysfs file contains. Was this done for i18n?

No, I changed it to string to enum because this is separating the actual
input from sysfs to the internal representation of a valid state. The
original patch read the sysfs file and compared string everywhere, this
is not a good practice. i18n is not done anywhere in progs so that's not
the reason.

Re: [PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[Goldwyn Rodrigues]

On  6:50 10/04, Anand Jain wrote:
> On 09/04/2021 23:56, Goldwyn Rodrigues wrote:
> > check_running_fs_exclop() can return 1 when exclop is changed to "none"
> > The ret is set by the return value of the select() operation. Checking
> > the exclusive op changes just the exclop variable while ret is still
> > set to 1.
> > 
> > Set ret exclusively if exclop is set to BTRFS_EXCL_NONE.
> > ---
> 
> SOB missing.

Yes, missed that.

> 
> >   common/utils.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/common/utils.c b/common/utils.c
> > index 57e41432..2e5175c3 100644
> > --- a/common/utils.c
> > +++ b/common/utils.c
> > @@ -2326,6 +2326,8 @@ int check_running_fs_exclop(int fd, enum exclusive_operation start, bool enqueue
> >   			tv.tv_sec /= 2;
> >   			ret = select(sysfs_fd + 1, NULL, NULL, &fds, &tv);
> >   			exclop = get_fs_exclop(fd);
> > +			if (exclop == BTRFS_EXCL_NONE)
> > +				ret = 0;
> >   			continue;
> >   		}
> >   	}
> > 
> 
> 
> This is bit inconsistent from what is done a few lines above:
> 
>         exclop = get_fs_exclop(fd);
>         if (exclop <= 0) {
>                 ret = 0;
>                 goto out;
>         }
> 
> We return 0 for both BTRFS_EXCLOP_NONE || BTRFS_EXCLOP_UNKNOWN.
> 

I am not sure why we are translating the sysfs file value to enums. The
only status we are concerned about is with "none", anything besides that
is considered to be busy, for code flow purposes. For error display, we
can print whatever the sysfs file contains. Was this done for i18n?

Of course, to maintain backward compatibility, we need to check on
existence of the file.

Anyways, I will re-post this patch with what is done a few lines above.

-- 
Goldwyn

Re: [PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[Anand Jain]

On 09/04/2021 23:56, Goldwyn Rodrigues wrote:
> check_running_fs_exclop() can return 1 when exclop is changed to "none"
> The ret is set by the return value of the select() operation. Checking
> the exclusive op changes just the exclop variable while ret is still
> set to 1.
> 
> Set ret exclusively if exclop is set to BTRFS_EXCL_NONE.
> ---

SOB missing.

>   common/utils.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/common/utils.c b/common/utils.c
> index 57e41432..2e5175c3 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -2326,6 +2326,8 @@ int check_running_fs_exclop(int fd, enum exclusive_operation start, bool enqueue
>   			tv.tv_sec /= 2;
>   			ret = select(sysfs_fd + 1, NULL, NULL, &fds, &tv);
>   			exclop = get_fs_exclop(fd);
> +			if (exclop == BTRFS_EXCL_NONE)
> +				ret = 0;
>   			continue;
>   		}
>   	}
> 


This is bit inconsistent from what is done a few lines above:

         exclop = get_fs_exclop(fd);
         if (exclop <= 0) {
                 ret = 0;
                 goto out;
         }

We return 0 for both BTRFS_EXCLOP_NONE || BTRFS_EXCLOP_UNKNOWN.

Thanks, Anand

[PATCH] btrfs-progs: Correct check_running_fs_exclop() return value

[Goldwyn Rodrigues]

check_running_fs_exclop() can return 1 when exclop is changed to "none"
The ret is set by the return value of the select() operation. Checking
the exclusive op changes just the exclop variable while ret is still
set to 1.

Set ret exclusively if exclop is set to BTRFS_EXCL_NONE.
---
 common/utils.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/utils.c b/common/utils.c
index 57e41432..2e5175c3 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -2326,6 +2326,8 @@ int check_running_fs_exclop(int fd, enum exclusive_operation start, bool enqueue
 			tv.tv_sec /= 2;
 			ret = select(sysfs_fd + 1, NULL, NULL, &fds, &tv);
 			exclop = get_fs_exclop(fd);
+			if (exclop == BTRFS_EXCL_NONE)
+				ret = 0;
 			continue;
 		}
 	}
-- 
2.30.2