[PATCH] nfsrahead: modify get_device_info logic

[PATCH] nfsrahead: modify get_device_info logic

[tbecker]

From: Thiago Becker <tbecker@redhat.com>

There are a few reports of failures by nfsrahead to set the read ahead
when the nfs mount information is not available when the udev event
fires. This was alleviated by retrying to read mountinfo multiple times,
but some cases where still failing to find the device information. To
further alleviate this issue, this patch adds a 50ms delay between
attempts. To not incur into unecessary delays, the logic in
get_device_info is reworked.

While we are in this, remove a second loop of reptitions of
get_device_info.

Signed-off-by: Thiago Becker <tbecker@redhat.com>
---
 tools/nfsrahead/main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/nfsrahead/main.c b/tools/nfsrahead/main.c
index 8a11cf1a..b7b889ff 100644
--- a/tools/nfsrahead/main.c
+++ b/tools/nfsrahead/main.c
@@ -117,9 +117,11 @@ out_free_device_info:
 
 static int get_device_info(const char *device_number, struct device_info *device_info)
 {
-	int ret = ENOENT;
-	for (int retry_count = 0; retry_count < 10 && ret != 0; retry_count++)
+	int ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
+	for (int retry_count = 0; retry_count < 5 && ret != 0; retry_count++) {
+		usleep(50000);
 		ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
+	}
 
 	return ret;
 }
@@ -135,7 +137,7 @@ static int conf_get_readahead(const char *kind) {
 
 int main(int argc, char **argv)
 {
-	int ret = 0, retry, opt;
+	int ret = 0, opt;
 	struct device_info device;
 	unsigned int readahead = 128, log_level, log_stderr = 0;
 
@@ -163,11 +165,7 @@ int main(int argc, char **argv)
 	if ((argc - optind) != 1)
 		xlog_err("expected the device number of a BDI; is udev ok?");
 
-	for (retry = 0; retry <= 10; retry++ )
-		if ((ret = get_device_info(argv[optind], &device)) == 0)
-			break;
-
-	if (ret != 0 || device.fstype == NULL) {
+	if ((ret = get_device_info(argv[optind], &device)) != 0 || device.fstype == NULL) {
 		xlog(D_GENERAL, "unable to find device %s\n", argv[optind]);
 		goto out;
 	}
-- 
2.43.5

Re: [PATCH] nfsrahead: modify get_device_info logic

[Steve Dickson]

Hello,

On 8/12/25 5:38 PM, tbecker@redhat.com wrote:
> From: Thiago Becker <tbecker@redhat.com>
> 
> There are a few reports of failures by nfsrahead to set the read ahead
> when the nfs mount information is not available when the udev event
> fires. This was alleviated by retrying to read mountinfo multiple times,
> but some cases where still failing to find the device information. To
> further alleviate this issue, this patch adds a 50ms delay between
> attempts. To not incur into unecessary delays, the logic in
> get_device_info is reworked.
> 
> While we are in this, remove a second loop of reptitions of
> get_device_info.
> 
> Signed-off-by: Thiago Becker <tbecker@redhat.com>
> ---
>   tools/nfsrahead/main.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/nfsrahead/main.c b/tools/nfsrahead/main.c
> index 8a11cf1a..b7b889ff 100644
> --- a/tools/nfsrahead/main.c
> +++ b/tools/nfsrahead/main.c
> @@ -117,9 +117,11 @@ out_free_device_info:
>   
>   static int get_device_info(const char *device_number, struct device_info *device_info)
>   {
> -	int ret = ENOENT;
> -	for (int retry_count = 0; retry_count < 10 && ret != 0; retry_count++)
> +	int ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
> +	for (int retry_count = 0; retry_count < 5 && ret != 0; retry_count++) {
> +		usleep(50000);
>   		ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
> +	}
get_mountinfo() will error out with an errno value (ret != 0) so
how is how are error detected? What am I missing...

Also why was a value of 50000 used to sleep?

steved.

>   
>   	return ret;
>   }
> @@ -135,7 +137,7 @@ static int conf_get_readahead(const char *kind) {
>   
>   int main(int argc, char **argv)
>   {
> -	int ret = 0, retry, opt;
> +	int ret = 0, opt;
>   	struct device_info device;
>   	unsigned int readahead = 128, log_level, log_stderr = 0;
>   
> @@ -163,11 +165,7 @@ int main(int argc, char **argv)
>   	if ((argc - optind) != 1)
>   		xlog_err("expected the device number of a BDI; is udev ok?");
>   
> -	for (retry = 0; retry <= 10; retry++ )
> -		if ((ret = get_device_info(argv[optind], &device)) == 0)
> -			break;
> -
> -	if (ret != 0 || device.fstype == NULL) {
> +	if ((ret = get_device_info(argv[optind], &device)) != 0 || device.fstype == NULL) {
>   		xlog(D_GENERAL, "unable to find device %s\n", argv[optind]);
>   		goto out;
>   	}

Re: [PATCH] nfsrahead: modify get_device_info logic

[Thiago Becker]

The success is detected in the loop in get_device_info, in the
stopping clause: if the call to get_mountinfo succeeds, the loop
stops, and the function returns success. If the call fails for the
number of retries plus one, the last error is returned, detected in
the caller, which logs an error.

The 50000 us is arbitrary. We need to give the kernel some time to
update mountinfo, and 50 ms is a value that works well, reducing the
number of attempts to 2 to succeed. The current implementation tries
for 50 times and fails because it executes too fast. Using a larger
value doesn't improve the success rate, and a smaller value increases
the number of retries (at least on my PC).

-- Thiago


On Mon, Aug 18, 2025 at 10:03 AM Steve Dickson <steved@redhat.com> wrote:
>
> Hello,
>
> On 8/12/25 5:38 PM, tbecker@redhat.com wrote:
> > From: Thiago Becker <tbecker@redhat.com>
> >
> > There are a few reports of failures by nfsrahead to set the read ahead
> > when the nfs mount information is not available when the udev event
> > fires. This was alleviated by retrying to read mountinfo multiple times,
> > but some cases where still failing to find the device information. To
> > further alleviate this issue, this patch adds a 50ms delay between
> > attempts. To not incur into unecessary delays, the logic in
> > get_device_info is reworked.
> >
> > While we are in this, remove a second loop of reptitions of
> > get_device_info.
> >
> > Signed-off-by: Thiago Becker <tbecker@redhat.com>
> > ---
> >   tools/nfsrahead/main.c | 14 ++++++--------
> >   1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/nfsrahead/main.c b/tools/nfsrahead/main.c
> > index 8a11cf1a..b7b889ff 100644
> > --- a/tools/nfsrahead/main.c
> > +++ b/tools/nfsrahead/main.c
> > @@ -117,9 +117,11 @@ out_free_device_info:
> >
> >   static int get_device_info(const char *device_number, struct device_info *device_info)
> >   {
> > -     int ret = ENOENT;
> > -     for (int retry_count = 0; retry_count < 10 && ret != 0; retry_count++)
> > +     int ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
> > +     for (int retry_count = 0; retry_count < 5 && ret != 0; retry_count++) {
> > +             usleep(50000);
> >               ret = get_mountinfo(device_number, device_info, MOUNTINFO_PATH);
> > +     }
> get_mountinfo() will error out with an errno value (ret != 0) so
> how is how are error detected? What am I missing...
>
> Also why was a value of 50000 used to sleep?
>
> steved.
>
> >
> >       return ret;
> >   }
> > @@ -135,7 +137,7 @@ static int conf_get_readahead(const char *kind) {
> >
> >   int main(int argc, char **argv)
> >   {
> > -     int ret = 0, retry, opt;
> > +     int ret = 0, opt;
> >       struct device_info device;
> >       unsigned int readahead = 128, log_level, log_stderr = 0;
> >
> > @@ -163,11 +165,7 @@ int main(int argc, char **argv)
> >       if ((argc - optind) != 1)
> >               xlog_err("expected the device number of a BDI; is udev ok?");
> >
> > -     for (retry = 0; retry <= 10; retry++ )
> > -             if ((ret = get_device_info(argv[optind], &device)) == 0)
> > -                     break;
> > -
> > -     if (ret != 0 || device.fstype == NULL) {
> > +     if ((ret = get_device_info(argv[optind], &device)) != 0 || device.fstype == NULL) {
> >               xlog(D_GENERAL, "unable to find device %s\n", argv[optind]);
> >               goto out;
> >       }
>


-- 
Thiago Rafael Becker
Software Maintenance Engineer
redhat.com