question about pnfs logic
diff mbox series

Message ID CAN-5tyG+ueNJapc-JGRsi=Tr5rKY6QxrHrouWgVo=6j8p5BeAA@mail.gmail.com
State New
Headers show
Series
  • question about pnfs logic
Related show

Commit Message

Olga Kornievskaia April 30, 2019, 9:25 p.m. UTC
Hi Trond,

I'm trying to figure existing code in order to fix a problem. Can you
please help?

Say there was an IO to the DS and it timeouts. Pnfs code in the
filelayout_async_handle_error() marks the device unavailable and marks
the layout to be returned. Timed out IO is retried against the MDS.
The new IO tries to do the pnfs and because the device is marked
unavailable (returning EINVAL) it propagates back all the way to the
pg_init setup and fails the IO with EINVAL. But IO shouldn't fail.

My question is why are we returning the status of the
filelayout_check_deviceid(), I propose that instead we at that point
set lseg=NULL and then the IO would be redirected to the MDS.

Or maybe I'm missing something else?

My proposed fix would be:

Comments

Trond Myklebust May 1, 2019, 11:10 a.m. UTC | #1
Hi Olga,

On Tue, 2019-04-30 at 17:25 -0400, Olga Kornievskaia wrote:
> Hi Trond,
> 
> I'm trying to figure existing code in order to fix a problem. Can you
> please help?
> 
> Say there was an IO to the DS and it timeouts. Pnfs code in the
> filelayout_async_handle_error() marks the device unavailable and
> marks
> the layout to be returned. Timed out IO is retried against the MDS.
> The new IO tries to do the pnfs and because the device is marked
> unavailable (returning EINVAL) it propagates back all the way to the
> pg_init setup and fails the IO with EINVAL. But IO shouldn't fail.
> 
> My question is why are we returning the status of the
> filelayout_check_deviceid(), I propose that instead we at that point
> set lseg=NULL and then the IO would be redirected to the MDS.
> 
> Or maybe I'm missing something else?
> 
> My proposed fix would be:
> diff --git a/fs/nfs/filelayout/filelayout.c
> b/fs/nfs/filelayout/filelayout.c
> index 61f46facb39c..b3e8ba3bd654 100644
> --- a/fs/nfs/filelayout/filelayout.c
> +++ b/fs/nfs/filelayout/filelayout.c
> @@ -904,7 +904,7 @@ fl_pnfs_update_layout(struct inode *ino,
>         status = filelayout_check_deviceid(lo, fl, gfp_flags);
>         if (status) {
>                 pnfs_put_lseg(lseg);
> -               lseg = ERR_PTR(status);
> +               lseg = NULL;
>         }
>  out:
>         return lseg;

I fully agree with your assessment. The objective of the code there
should indeed be to fall back to doing I/O through the MDS rather than
failing it altogether.

Given that the 5.2 merge window is likely to open next week, and that
we haven't seen any urgent reports of this bug before now, I suggest
that you add the signed-off-by line to the above patch and send it to
Anna for inclusion in that tree.
Please also add a 'Fixes:' line so that the patch can be automatically
backported to older kernel.

Cheers
  Trond
Olga Kornievskaia May 2, 2019, 12:41 p.m. UTC | #2
On Wed, May 1, 2019 at 7:10 AM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> Hi Olga,
>
> On Tue, 2019-04-30 at 17:25 -0400, Olga Kornievskaia wrote:
> > Hi Trond,
> >
> > I'm trying to figure existing code in order to fix a problem. Can you
> > please help?
> >
> > Say there was an IO to the DS and it timeouts. Pnfs code in the
> > filelayout_async_handle_error() marks the device unavailable and
> > marks
> > the layout to be returned. Timed out IO is retried against the MDS.
> > The new IO tries to do the pnfs and because the device is marked
> > unavailable (returning EINVAL) it propagates back all the way to the
> > pg_init setup and fails the IO with EINVAL. But IO shouldn't fail.
> >
> > My question is why are we returning the status of the
> > filelayout_check_deviceid(), I propose that instead we at that point
> > set lseg=NULL and then the IO would be redirected to the MDS.
> >
> > Or maybe I'm missing something else?
> >
> > My proposed fix would be:
> > diff --git a/fs/nfs/filelayout/filelayout.c
> > b/fs/nfs/filelayout/filelayout.c
> > index 61f46facb39c..b3e8ba3bd654 100644
> > --- a/fs/nfs/filelayout/filelayout.c
> > +++ b/fs/nfs/filelayout/filelayout.c
> > @@ -904,7 +904,7 @@ fl_pnfs_update_layout(struct inode *ino,
> >         status = filelayout_check_deviceid(lo, fl, gfp_flags);
> >         if (status) {
> >                 pnfs_put_lseg(lseg);
> > -               lseg = ERR_PTR(status);
> > +               lseg = NULL;
> >         }
> >  out:
> >         return lseg;
>
> I fully agree with your assessment. The objective of the code there
> should indeed be to fall back to doing I/O through the MDS rather than
> failing it altogether.
>
> Given that the 5.2 merge window is likely to open next week, and that
> we haven't seen any urgent reports of this bug before now, I suggest
> that you add the signed-off-by line to the above patch and send it to
> Anna for inclusion in that tree.
> Please also add a 'Fixes:' line so that the patch can be automatically
> backported to older kernel.

Thanks Trond for the confirmation. I'll do this as soon as I'll chase
down one more element that I can't explain in this failing scenario.

>
> Cheers
>   Trond
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>

Patch
diff mbox series

diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c
index 61f46facb39c..b3e8ba3bd654 100644
--- a/fs/nfs/filelayout/filelayout.c
+++ b/fs/nfs/filelayout/filelayout.c
@@ -904,7 +904,7 @@  fl_pnfs_update_layout(struct inode *ino,
        status = filelayout_check_deviceid(lo, fl, gfp_flags);
        if (status) {
                pnfs_put_lseg(lseg);
-               lseg = ERR_PTR(status);
+               lseg = NULL;
        }
 out:
        return lseg;