| Message ID | 712798c489f69e39f235077f6ca85a7c133eeb58.1459949813.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > I'm not exactly sure this is the safest thing to do since you can pass > -oremount on the first mount and skip option validation. Maybe someone with > better insight into the mount paths could comment. Does mount need some > refactoring? Its logic seems arcane.. and I think there is a lot of dead > code. It's arcane because NFS mounting has a lot of corner cases that have evolved over the years. If you have an example of dead code, can you post it? > This is very quick attempt to fix > https://bugzilla.redhat.com/show_bug.cgi?id=1313550 "You are not authorized to access bug #1313550." I'm guessing you want to use the existing addr= option on a remount in case the DNS resolution returns a different address. I'm uncertain why a remount should succeed in this case: if the server has a different IP address, how was the mount working at all? > 8<------------------------------------------------------------------------- > > A remount might fail if name resolution returns a different server address > for the mount. Since we've already validated the options the first time, > skip validation if remounting. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > utils/mount/stropts.c | 6 +++--- > 1 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 86829a9..9383bb4 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { > > static int nfsmount_start(struct nfsmount_info *mi) > { > - if (!nfs_validate_options(mi)) > - return EX_FAIL; > - > /* > * Avoid retry and negotiation logic when remounting > */ > if (mi->flags & MS_REMOUNT) > return nfs_remount(mi); > > + if (!nfs_validate_options(mi)) > + return EX_FAIL; > + > if (po_rightmost(mi->options, nfs_background_opttbl) == 0) > return nfsmount_bg(mi); > else > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 6 Apr 2016, Chuck Lever wrote: > > > On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > > > I'm not exactly sure this is the safest thing to do since you can pass > > -oremount on the first mount and skip option validation. Maybe someone with > > better insight into the mount paths could comment. Does mount need some > > refactoring? Its logic seems arcane.. and I think there is a lot of dead > > code. > > It's arcane because NFS mounting has a lot of corner cases > that have evolved over the years. If you have an example of > dead code, can you post it? Absolutely. I don't have it ready right now, but I do remember coming across some sections several times and wondering how they could be used. I need to do a better job of posting when I find things instead of putting them off and forgetting about them. > > This is very quick attempt to fix > > https://bugzilla.redhat.com/show_bug.cgi?id=1313550 > > "You are not authorized to access bug #1313550." Sorry. I've just tried to fix that and I cannot. The basic info there is that kdump always tries to remount,rw a target.. and that is breaking on NFS. The bug doesn't really provide anything more useful to the discussion other than maybe help Steved find the original problem. > I'm guessing you want to use the existing addr= option on a > remount in case the DNS resolution returns a different address. Right. > I'm uncertain why a remount should succeed in this case: if > the server has a different IP address, how was the mount working > at all? If the server has multiple A or AAAA records, and the the results are returned round-robin style, we can end up with a different address for the server. Ben > > 8<------------------------------------------------------------------------- > > > > A remount might fail if name resolution returns a different server address > > for the mount. Since we've already validated the options the first time, > > skip validation if remounting. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > utils/mount/stropts.c | 6 +++--- > > 1 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > > index 86829a9..9383bb4 100644 > > --- a/utils/mount/stropts.c > > +++ b/utils/mount/stropts.c > > @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { > > > > static int nfsmount_start(struct nfsmount_info *mi) > > { > > - if (!nfs_validate_options(mi)) > > - return EX_FAIL; > > - > > /* > > * Avoid retry and negotiation logic when remounting > > */ > > if (mi->flags & MS_REMOUNT) > > return nfs_remount(mi); > > > > + if (!nfs_validate_options(mi)) > > + return EX_FAIL; > > + > > if (po_rightmost(mi->options, nfs_background_opttbl) == 0) > > return nfsmount_bg(mi); > > else > > -- > > 1.7.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Apr 6, 2016, at 8:24 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > On Wed, 6 Apr 2016, Chuck Lever wrote: >> >>> On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: >>> >>> I'm not exactly sure this is the safest thing to do since you can pass >>> -oremount on the first mount and skip option validation. Maybe someone with >>> better insight into the mount paths could comment. Does mount need some >>> refactoring? Its logic seems arcane.. and I think there is a lot of dead >>> code. >> >> It's arcane because NFS mounting has a lot of corner cases >> that have evolved over the years. If you have an example of >> dead code, can you post it? > > Absolutely. I don't have it ready right now, but I do remember coming across > some sections several times and wondering how they could be used. > > I need to do a better job of posting when I find things instead of putting > them off and forgetting about them. > >>> This is very quick attempt to fix >>> https://bugzilla.redhat.com/show_bug.cgi?id=1313550 >> >> "You are not authorized to access bug #1313550." > > Sorry. I've just tried to fix that and I cannot. The basic info there is > that kdump always tries to remount,rw a target.. and that is breaking on > NFS. Always breaking? Or just in the case where the server has multiple homes? > The bug doesn't really provide anything more useful to the discussion > other than maybe help Steved find the original problem. > >> I'm guessing you want to use the existing addr= option on a >> remount in case the DNS resolution returns a different address. > > Right. > >> I'm uncertain why a remount should succeed in this case: if >> the server has a different IP address, how was the mount working >> at all? > > If the server has multiple A or AAAA records, and the the results are > returned round-robin style, we can end up with a different address for > the server. I agree that a second DNS resolution here is probably not helpful or needed. Still, multi-home NFS seems like a crap shoot to begin with. Maybe I'm just not awake yet. But I think you do need to validate mount options on a remount: otherwise you can pass "-o remount,garbage". Or did I misunderstand? > Ben > >>> 8<------------------------------------------------------------------------- >>> >>> A remount might fail if name resolution returns a different server address >>> for the mount. Since we've already validated the options the first time, >>> skip validation if remounting. >>> >>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> >>> --- >>> utils/mount/stropts.c | 6 +++--- >>> 1 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c >>> index 86829a9..9383bb4 100644 >>> --- a/utils/mount/stropts.c >>> +++ b/utils/mount/stropts.c >>> @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { >>> >>> static int nfsmount_start(struct nfsmount_info *mi) >>> { >>> - if (!nfs_validate_options(mi)) >>> - return EX_FAIL; >>> - >>> /* >>> * Avoid retry and negotiation logic when remounting >>> */ >>> if (mi->flags & MS_REMOUNT) >>> return nfs_remount(mi); >>> >>> + if (!nfs_validate_options(mi)) >>> + return EX_FAIL; >>> + >>> if (po_rightmost(mi->options, nfs_background_opttbl) == 0) >>> return nfsmount_bg(mi); >>> else >>> -- >>> 1.7.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> Chuck Lever -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 6 Apr 2016, Chuck Lever wrote: > > > On Apr 6, 2016, at 8:24 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > > > On Wed, 6 Apr 2016, Chuck Lever wrote: > >> > >>> On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > >>> > >>> I'm not exactly sure this is the safest thing to do since you can pass > >>> -oremount on the first mount and skip option validation. Maybe someone with > >>> better insight into the mount paths could comment. Does mount need some > >>> refactoring? Its logic seems arcane.. and I think there is a lot of dead > >>> code. > >> > >> It's arcane because NFS mounting has a lot of corner cases > >> that have evolved over the years. If you have an example of > >> dead code, can you post it? > > > > Absolutely. I don't have it ready right now, but I do remember coming across > > some sections several times and wondering how they could be used. > > > > I need to do a better job of posting when I find things instead of putting > > them off and forgetting about them. > > > >>> This is very quick attempt to fix > >>> https://bugzilla.redhat.com/show_bug.cgi?id=1313550 > >> > >> "You are not authorized to access bug #1313550." > > > > Sorry. I've just tried to fix that and I cannot. The basic info there is > > that kdump always tries to remount,rw a target.. and that is breaking on > > NFS. > > Always breaking? Or just in the case where the server has > multiple homes? In the case of multiple records. > > The bug doesn't really provide anything more useful to the discussion > > other than maybe help Steved find the original problem. > > > >> I'm guessing you want to use the existing addr= option on a > >> remount in case the DNS resolution returns a different address. > > > > Right. > > > >> I'm uncertain why a remount should succeed in this case: if > >> the server has a different IP address, how was the mount working > >> at all? > > > > If the server has multiple A or AAAA records, and the the results are > > returned round-robin style, we can end up with a different address for > > the server. > > I agree that a second DNS resolution here is probably not > helpful or needed. Still, multi-home NFS seems like a crap > shoot to begin with. Maybe I'm just not awake yet. It is a crap-shoot. But I think I should try to give a multi-homed server a shot at serving NFS.. In this case, the server may not actually even respond on the additional addresses - they just resolve. > But I think you do need to validate mount options on a > remount: otherwise you can pass "-o remount,garbage". Or > did I misunderstand? No, I don't think you did.. and that's the second opinion I'd sorta expected. So thanks for looking at this, and I'll re-spin it to do it the non-lazy way. Ben > > > >>> 8<------------------------------------------------------------------------- > >>> > >>> A remount might fail if name resolution returns a different server address > >>> for the mount. Since we've already validated the options the first time, > >>> skip validation if remounting. > >>> > >>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > >>> --- > >>> utils/mount/stropts.c | 6 +++--- > >>> 1 files changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > >>> index 86829a9..9383bb4 100644 > >>> --- a/utils/mount/stropts.c > >>> +++ b/utils/mount/stropts.c > >>> @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { > >>> > >>> static int nfsmount_start(struct nfsmount_info *mi) > >>> { > >>> - if (!nfs_validate_options(mi)) > >>> - return EX_FAIL; > >>> - > >>> /* > >>> * Avoid retry and negotiation logic when remounting > >>> */ > >>> if (mi->flags & MS_REMOUNT) > >>> return nfs_remount(mi); > >>> > >>> + if (!nfs_validate_options(mi)) > >>> + return EX_FAIL; > >>> + > >>> if (po_rightmost(mi->options, nfs_background_opttbl) == 0) > >>> return nfsmount_bg(mi); > >>> else > >>> -- > >>> 1.7.1 > >>> > >>> -- > >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > >>> the body of a message to majordomo@vger.kernel.org > >>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >> -- > >> Chuck Lever > > -- > Chuck Lever > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Apr 6, 2016, at 8:47 AM, Chuck Lever <chuck.lever@oracle.com> wrote: > >> >> On Apr 6, 2016, at 8:24 AM, Benjamin Coddington <bcodding@redhat.com> wrote: >> >> On Wed, 6 Apr 2016, Chuck Lever wrote: >>> >>>> On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: >>>> >>>> I'm not exactly sure this is the safest thing to do since you can pass >>>> -oremount on the first mount and skip option validation. Maybe someone with >>>> better insight into the mount paths could comment. Does mount need some >>>> refactoring? Its logic seems arcane.. and I think there is a lot of dead >>>> code. >>> >>> It's arcane because NFS mounting has a lot of corner cases >>> that have evolved over the years. If you have an example of >>> dead code, can you post it? >> >> Absolutely. I don't have it ready right now, but I do remember coming across >> some sections several times and wondering how they could be used. >> >> I need to do a better job of posting when I find things instead of putting >> them off and forgetting about them. >> >>>> This is very quick attempt to fix >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1313550 >>> >>> "You are not authorized to access bug #1313550." >> >> Sorry. I've just tried to fix that and I cannot. The basic info there is >> that kdump always tries to remount,rw a target.. and that is breaking on >> NFS. > > Always breaking? Or just in the case where the server has > multiple homes? > > >> The bug doesn't really provide anything more useful to the discussion >> other than maybe help Steved find the original problem. >> >>> I'm guessing you want to use the existing addr= option on a >>> remount in case the DNS resolution returns a different address. >> >> Right. >> >>> I'm uncertain why a remount should succeed in this case: if >>> the server has a different IP address, how was the mount working >>> at all? >> >> If the server has multiple A or AAAA records, and the the results are >> returned round-robin style, we can end up with a different address for >> the server. > > I agree that a second DNS resolution here is probably not > helpful or needed. Still, multi-home NFS seems like a crap > shoot to begin with. Maybe I'm just not awake yet. I should clarify: I mean round-robin here, not multi-home. Is this only a problem when the server has an A and an AAAA address, and there's no "proto=" specified? > But I think you do need to validate mount options on a > remount: otherwise you can pass "-o remount,garbage". Or > did I misunderstand? > > >> Ben >> >>>> 8<------------------------------------------------------------------------- >>>> >>>> A remount might fail if name resolution returns a different server address >>>> for the mount. Since we've already validated the options the first time, >>>> skip validation if remounting. >>>> >>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> >>>> --- >>>> utils/mount/stropts.c | 6 +++--- >>>> 1 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c >>>> index 86829a9..9383bb4 100644 >>>> --- a/utils/mount/stropts.c >>>> +++ b/utils/mount/stropts.c >>>> @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { >>>> >>>> static int nfsmount_start(struct nfsmount_info *mi) >>>> { >>>> - if (!nfs_validate_options(mi)) >>>> - return EX_FAIL; >>>> - >>>> /* >>>> * Avoid retry and negotiation logic when remounting >>>> */ >>>> if (mi->flags & MS_REMOUNT) >>>> return nfs_remount(mi); >>>> >>>> + if (!nfs_validate_options(mi)) >>>> + return EX_FAIL; >>>> + >>>> if (po_rightmost(mi->options, nfs_background_opttbl) == 0) >>>> return nfsmount_bg(mi); >>>> else >>>> -- >>>> 1.7.1 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> -- >>> Chuck Lever > > -- > Chuck Lever > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 6 Apr 2016, Chuck Lever wrote: > > > On Apr 6, 2016, at 8:47 AM, Chuck Lever <chuck.lever@oracle.com> wrote: > > > >> > >> On Apr 6, 2016, at 8:24 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > >> > >> On Wed, 6 Apr 2016, Chuck Lever wrote: > >>> > >>>> On Apr 6, 2016, at 6:41 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > >>>> > >>>> I'm not exactly sure this is the safest thing to do since you can pass > >>>> -oremount on the first mount and skip option validation. Maybe someone with > >>>> better insight into the mount paths could comment. Does mount need some > >>>> refactoring? Its logic seems arcane.. and I think there is a lot of dead > >>>> code. > >>> > >>> It's arcane because NFS mounting has a lot of corner cases > >>> that have evolved over the years. If you have an example of > >>> dead code, can you post it? > >> > >> Absolutely. I don't have it ready right now, but I do remember coming across > >> some sections several times and wondering how they could be used. > >> > >> I need to do a better job of posting when I find things instead of putting > >> them off and forgetting about them. > >> > >>>> This is very quick attempt to fix > >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1313550 > >>> > >>> "You are not authorized to access bug #1313550." > >> > >> Sorry. I've just tried to fix that and I cannot. The basic info there is > >> that kdump always tries to remount,rw a target.. and that is breaking on > >> NFS. > > > > Always breaking? Or just in the case where the server has > > multiple homes? > > > > > >> The bug doesn't really provide anything more useful to the discussion > >> other than maybe help Steved find the original problem. > >> > >>> I'm guessing you want to use the existing addr= option on a > >>> remount in case the DNS resolution returns a different address. > >> > >> Right. > >> > >>> I'm uncertain why a remount should succeed in this case: if > >>> the server has a different IP address, how was the mount working > >>> at all? > >> > >> If the server has multiple A or AAAA records, and the the results are > >> returned round-robin style, we can end up with a different address for > >> the server. > > > > I agree that a second DNS resolution here is probably not > > helpful or needed. Still, multi-home NFS seems like a crap > > shoot to begin with. Maybe I'm just not awake yet. > > I should clarify: I mean round-robin here, not multi-home. > > Is this only a problem when the server has an A and an > AAAA address, and there's no "proto=" specified? In my testing it was a problem when returning multiple A records round-robin. I think the same thing would be true of multiple AAAA records. I don't think this problem exists with an A record and an AAAA record because getaddrinfo doesn't group them - they are different families. Ben > > But I think you do need to validate mount options on a > > remount: otherwise you can pass "-o remount,garbage". Or > > did I misunderstand? > > > > > >> Ben > >> > >>>> 8<------------------------------------------------------------------------- > >>>> > >>>> A remount might fail if name resolution returns a different server address > >>>> for the mount. Since we've already validated the options the first time, > >>>> skip validation if remounting. > >>>> > >>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > >>>> --- > >>>> utils/mount/stropts.c | 6 +++--- > >>>> 1 files changed, 3 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > >>>> index 86829a9..9383bb4 100644 > >>>> --- a/utils/mount/stropts.c > >>>> +++ b/utils/mount/stropts.c > >>>> @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { > >>>> > >>>> static int nfsmount_start(struct nfsmount_info *mi) > >>>> { > >>>> - if (!nfs_validate_options(mi)) > >>>> - return EX_FAIL; > >>>> - > >>>> /* > >>>> * Avoid retry and negotiation logic when remounting > >>>> */ > >>>> if (mi->flags & MS_REMOUNT) > >>>> return nfs_remount(mi); > >>>> > >>>> + if (!nfs_validate_options(mi)) > >>>> + return EX_FAIL; > >>>> + > >>>> if (po_rightmost(mi->options, nfs_background_opttbl) == 0) > >>>> return nfsmount_bg(mi); > >>>> else > >>>> -- > >>>> 1.7.1 > >>>> > >>>> -- > >>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > >>>> the body of a message to majordomo@vger.kernel.org > >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html > >>> > >>> -- > >>> Chuck Lever > > > > -- > > Chuck Lever > > > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Chuck Lever > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c index 86829a9..9383bb4 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -1090,15 +1090,15 @@ static const char *nfs_background_opttbl[] = { static int nfsmount_start(struct nfsmount_info *mi) { - if (!nfs_validate_options(mi)) - return EX_FAIL; - /* * Avoid retry and negotiation logic when remounting */ if (mi->flags & MS_REMOUNT) return nfs_remount(mi); + if (!nfs_validate_options(mi)) + return EX_FAIL; + if (po_rightmost(mi->options, nfs_background_opttbl) == 0) return nfsmount_bg(mi); else
I'm not exactly sure this is the safest thing to do since you can pass -oremount on the first mount and skip option validation. Maybe someone with better insight into the mount paths could comment. Does mount need some refactoring? Its logic seems arcane.. and I think there is a lot of dead code. This is very quick attempt to fix https://bugzilla.redhat.com/show_bug.cgi?id=1313550 8<------------------------------------------------------------------------- A remount might fail if name resolution returns a different server address for the mount. Since we've already validated the options the first time, skip validation if remounting. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- utils/mount/stropts.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)