| Message ID | 20230119175010.57814-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] pNFS/filelayout: treat GETDEVICEINFO errors as LAYOUTUNAVAILABLE | expand |
> On Jan 19, 2023, at 12:50, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > If the call to GETDEVICEINFO fails, the client fallback to doing IO > to MDS but on every new IO call, the client tries to get the device > again. Instead, mark the layout as unavailable as well. This way > the client will re-try after a timeout period. > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > fs/nfs/filelayout/filelayout.c | 1 + > fs/nfs/pnfs.c | 7 +++++++ > fs/nfs/pnfs.h | 2 ++ > 3 files changed, 10 insertions(+) > > diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c > index 4974cd18ca46..13df85457cf5 100644 > --- a/fs/nfs/filelayout/filelayout.c > +++ b/fs/nfs/filelayout/filelayout.c > @@ -862,6 +862,7 @@ fl_pnfs_update_layout(struct inode *ino, > > status = filelayout_check_deviceid(lo, fl, gfp_flags); > if (status) { > + pnfs_mark_layout_unavailable(lo, iomode); > pnfs_put_lseg(lseg); > lseg = NULL; > } > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index a5db5158c634..bac15dcf99bb 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -491,6 +491,13 @@ pnfs_layout_set_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) > refcount_inc(&lo->plh_refcount); > } > > +void > +pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, enum pnfs_iomode fail_bit) > +{ > + pnfs_layout_set_fail_bit(lo, pnfs_iomode_to_fail_bit(fail_bit)); I suggest rather using pnfs_layout_io_set_failed() so that we also evict the layout segment that references this unrecognised deviceid. In fact, there is already an exported function pnfs_set_lo_fail() (which could definitely do with a better name!) that does this. > +} > +EXPORT_SYMBOL_GPL(pnfs_mark_layout_unavailable); > + > static void > pnfs_layout_clear_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) > { > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > index e3e6a41f19de..9f47bd883fc3 100644 > --- a/fs/nfs/pnfs.h > +++ b/fs/nfs/pnfs.h > @@ -343,6 +343,8 @@ void pnfs_error_mark_layout_for_return(struct inode *inode, > void pnfs_layout_return_unused_byclid(struct nfs_client *clp, > enum pnfs_iomode iomode); > > +void pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, > + enum pnfs_iomode iomode); > /* nfs4_deviceid_flags */ > enum { > NFS_DEVICEID_INVALID = 0, /* set when MDS clientid recalled */ > -- > 2.31.1 >
On Thu, Jan 19, 2023 at 1:24 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > > > > On Jan 19, 2023, at 12:50, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > > > If the call to GETDEVICEINFO fails, the client fallback to doing IO > > to MDS but on every new IO call, the client tries to get the device > > again. Instead, mark the layout as unavailable as well. This way > > the client will re-try after a timeout period. > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > --- > > fs/nfs/filelayout/filelayout.c | 1 + > > fs/nfs/pnfs.c | 7 +++++++ > > fs/nfs/pnfs.h | 2 ++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c > > index 4974cd18ca46..13df85457cf5 100644 > > --- a/fs/nfs/filelayout/filelayout.c > > +++ b/fs/nfs/filelayout/filelayout.c > > @@ -862,6 +862,7 @@ fl_pnfs_update_layout(struct inode *ino, > > > > status = filelayout_check_deviceid(lo, fl, gfp_flags); > > if (status) { > > + pnfs_mark_layout_unavailable(lo, iomode); > > pnfs_put_lseg(lseg); > > lseg = NULL; > > } > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > > index a5db5158c634..bac15dcf99bb 100644 > > --- a/fs/nfs/pnfs.c > > +++ b/fs/nfs/pnfs.c > > @@ -491,6 +491,13 @@ pnfs_layout_set_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) > > refcount_inc(&lo->plh_refcount); > > } > > > > +void > > +pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, enum pnfs_iomode fail_bit) > > +{ > > + pnfs_layout_set_fail_bit(lo, pnfs_iomode_to_fail_bit(fail_bit)); > > I suggest rather using pnfs_layout_io_set_failed() so that we also evict the layout segment that references this unrecognised deviceid. In fact, there is already an exported function pnfs_set_lo_fail() (which could definitely do with a better name!) that does this. I'm not opposed to this approach. In the proposed patch, I treated it as the layout being still valid but unavailable. My question is: I think we need to return the layout before doing this, correct? Should I be making changes to export something like pnfs_set_plh_return_info() or would adding a new function to pnfs.c that does pnfs_set_lo_fail and returns the layout? something like +void pnfs_invalidate_return_layout(struct pnfs_layout_segment *lseg) +{ + pnfs_layout_io_set_failed(lseg->pls_layout, lseg->pls_range.iomode); + pnfs_set_plh_return_info(lseg->pls_layout, lseg->pls_range.iomode, 0); +} +EXPORT_SYMBOL_GPL(pnfs_invalidate_return_layout); status = filelayout_check_deviceid(lo, fl, gfp_flags); if (status) { + pnfs_invalidate_return_layout(lseg); pnfs_put_lseg(lseg); lseg = NULL; > > > +} > > +EXPORT_SYMBOL_GPL(pnfs_mark_layout_unavailable); > > + > > static void > > pnfs_layout_clear_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) > > { > > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h > > index e3e6a41f19de..9f47bd883fc3 100644 > > --- a/fs/nfs/pnfs.h > > +++ b/fs/nfs/pnfs.h > > @@ -343,6 +343,8 @@ void pnfs_error_mark_layout_for_return(struct inode *inode, > > void pnfs_layout_return_unused_byclid(struct nfs_client *clp, > > enum pnfs_iomode iomode); > > > > +void pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, > > + enum pnfs_iomode iomode); > > /* nfs4_deviceid_flags */ > > enum { > > NFS_DEVICEID_INVALID = 0, /* set when MDS clientid recalled */ > > -- > > 2.31.1 > > > > _________________________________ > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com >
> On Jan 19, 2023, at 14:40, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > On Thu, Jan 19, 2023 at 1:24 PM Trond Myklebust <trondmy@hammerspace.com> wrote: >> >> >> >>> On Jan 19, 2023, at 12:50, Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: >>> >>> If the call to GETDEVICEINFO fails, the client fallback to doing IO >>> to MDS but on every new IO call, the client tries to get the device >>> again. Instead, mark the layout as unavailable as well. This way >>> the client will re-try after a timeout period. >>> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>> --- >>> fs/nfs/filelayout/filelayout.c | 1 + >>> fs/nfs/pnfs.c | 7 +++++++ >>> fs/nfs/pnfs.h | 2 ++ >>> 3 files changed, 10 insertions(+) >>> >>> diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c >>> index 4974cd18ca46..13df85457cf5 100644 >>> --- a/fs/nfs/filelayout/filelayout.c >>> +++ b/fs/nfs/filelayout/filelayout.c >>> @@ -862,6 +862,7 @@ fl_pnfs_update_layout(struct inode *ino, >>> >>> status = filelayout_check_deviceid(lo, fl, gfp_flags); >>> if (status) { >>> + pnfs_mark_layout_unavailable(lo, iomode); >>> pnfs_put_lseg(lseg); >>> lseg = NULL; >>> } >>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c >>> index a5db5158c634..bac15dcf99bb 100644 >>> --- a/fs/nfs/pnfs.c >>> +++ b/fs/nfs/pnfs.c >>> @@ -491,6 +491,13 @@ pnfs_layout_set_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) >>> refcount_inc(&lo->plh_refcount); >>> } >>> >>> +void >>> +pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, enum pnfs_iomode fail_bit) >>> +{ >>> + pnfs_layout_set_fail_bit(lo, pnfs_iomode_to_fail_bit(fail_bit)); >> >> I suggest rather using pnfs_layout_io_set_failed() so that we also evict the layout segment that references this unrecognised deviceid. In fact, there is already an exported function pnfs_set_lo_fail() (which could definitely do with a better name!) that does this. > > I'm not opposed to this approach. In the proposed patch, I treated it > as the layout being still valid but unavailable. My question is: I > think we need to return the layout before doing this, correct? Should > I be making changes to export something like > pnfs_set_plh_return_info() or would adding a new function to pnfs.c > that does pnfs_set_lo_fail and returns the layout? I suggest rather changing the call to pnfs_mark_matching_lsegs_invalid() in pnfs_layout_io_set_failed() and make it call pnfs_mark_matching_lsegs_return() instead. > > something like > +void pnfs_invalidate_return_layout(struct pnfs_layout_segment *lseg) > +{ > + pnfs_layout_io_set_failed(lseg->pls_layout, lseg->pls_range.iomode); > + pnfs_set_plh_return_info(lseg->pls_layout, lseg->pls_range.iomode, 0); ^^^^^^^ this won’t suffice to invalidate the layout. > +} > +EXPORT_SYMBOL_GPL(pnfs_invalidate_return_layout); > > status = filelayout_check_deviceid(lo, fl, gfp_flags); > if (status) { > + pnfs_invalidate_return_layout(lseg); > pnfs_put_lseg(lseg); > lseg = NULL; > >> >>> +} >>> +EXPORT_SYMBOL_GPL(pnfs_mark_layout_unavailable); >>> + >>> static void >>> pnfs_layout_clear_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) >>> { >>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h >>> index e3e6a41f19de..9f47bd883fc3 100644 >>> --- a/fs/nfs/pnfs.h >>> +++ b/fs/nfs/pnfs.h >>> @@ -343,6 +343,8 @@ void pnfs_error_mark_layout_for_return(struct inode *inode, >>> void pnfs_layout_return_unused_byclid(struct nfs_client *clp, >>> enum pnfs_iomode iomode); >>> >>> +void pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, >>> + enum pnfs_iomode iomode); >>> /* nfs4_deviceid_flags */ >>> enum { >>> NFS_DEVICEID_INVALID = 0, /* set when MDS clientid recalled */ >>> -- >>> 2.31.1 >>>
diff --git a/fs/nfs/filelayout/filelayout.c b/fs/nfs/filelayout/filelayout.c index 4974cd18ca46..13df85457cf5 100644 --- a/fs/nfs/filelayout/filelayout.c +++ b/fs/nfs/filelayout/filelayout.c @@ -862,6 +862,7 @@ fl_pnfs_update_layout(struct inode *ino, status = filelayout_check_deviceid(lo, fl, gfp_flags); if (status) { + pnfs_mark_layout_unavailable(lo, iomode); pnfs_put_lseg(lseg); lseg = NULL; } diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index a5db5158c634..bac15dcf99bb 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -491,6 +491,13 @@ pnfs_layout_set_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) refcount_inc(&lo->plh_refcount); } +void +pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, enum pnfs_iomode fail_bit) +{ + pnfs_layout_set_fail_bit(lo, pnfs_iomode_to_fail_bit(fail_bit)); +} +EXPORT_SYMBOL_GPL(pnfs_mark_layout_unavailable); + static void pnfs_layout_clear_fail_bit(struct pnfs_layout_hdr *lo, int fail_bit) { diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h index e3e6a41f19de..9f47bd883fc3 100644 --- a/fs/nfs/pnfs.h +++ b/fs/nfs/pnfs.h @@ -343,6 +343,8 @@ void pnfs_error_mark_layout_for_return(struct inode *inode, void pnfs_layout_return_unused_byclid(struct nfs_client *clp, enum pnfs_iomode iomode); +void pnfs_mark_layout_unavailable(struct pnfs_layout_hdr *lo, + enum pnfs_iomode iomode); /* nfs4_deviceid_flags */ enum { NFS_DEVICEID_INVALID = 0, /* set when MDS clientid recalled */
If the call to GETDEVICEINFO fails, the client fallback to doing IO to MDS but on every new IO call, the client tries to get the device again. Instead, mark the layout as unavailable as well. This way the client will re-try after a timeout period. Signed-off-by: Olga Kornievskaia <kolga@netapp.com> --- fs/nfs/filelayout/filelayout.c | 1 + fs/nfs/pnfs.c | 7 +++++++ fs/nfs/pnfs.h | 2 ++ 3 files changed, 10 insertions(+)