diff mbox series

btrfs-progs: Correct check_running_fs_exclop() return value

Message ID 20210409155644.qkk6puelfjvtjwqs@fiona (mailing list archive)
State New
Headers show
Series btrfs-progs: Correct check_running_fs_exclop() return value | expand

Commit Message

Goldwyn Rodrigues April 9, 2021, 3:56 p.m. UTC
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(+)

Comments

Anand Jain April 9, 2021, 10:50 p.m. UTC | #1
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
Goldwyn Rodrigues April 12, 2021, 3:56 p.m. UTC | #2
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.
diff mbox series

Patch

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;
 		}
 	}