| Message ID | 87lgxiwoxi.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ping? No sign of this in linux-next, and no replies.... Thanks, NeilBrown On Fri, Oct 21 2016, NeilBrown wrote: > > Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL > 'ds', then ds->ds_clp will also be non-NULL. > > This is not necessasrily true in the case when the process received a > fatal signal while nfs4_pnfs_ds_connect is waiting in > nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the > devid may not recently have been marked unavailable. > > So add a test for ->ds_clp == NULL and return NULL in that case. > > Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") > Signed-off-by: NeilBrown <neilb@suse.com> > --- > fs/nfs/filelayout/filelayoutdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c > index 4946ef40ba87..85ef38f9765f 100644 > --- a/fs/nfs/filelayout/filelayoutdev.c > +++ b/fs/nfs/filelayout/filelayoutdev.c > @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) > s->nfs_client->cl_rpcclient->cl_auth->au_flavor); > > out_test_devid: > - if (filelayout_test_devid_unavailable(devid)) > + if (ret->ds_clp == NULL || > + filelayout_test_devid_unavailable(devid)) > ret = NULL; > out: > return ret; > -- > 2.10.1
DQo+IE9uIE5vdiAxOCwgMjAxNiwgYXQgMDA6MDEsIE5laWxCcm93biA8bmVpbGJAc3VzZS5jb20+ IHdyb3RlOg0KPiANCj4gDQo+IFBpbmc/DQo+IE5vIHNpZ24gb2YgdGhpcyBpbiBsaW51eC1uZXh0 LCBhbmQgbm8gcmVwbGllc+KApi4NCg0KSeKAmWQgbGlrZSBzb21lIGNvbmZpcm1hdGlvbiBmcm9t IHRoZSBOZXRBcHAgZm9sa3Mgb24gdGhpcy4gVGhleSBhcmUgdGhlIG1haW4gc3Rha2Vob2xkZXJz IGZvciB0aGUgcE5GUyBmaWxlcyBpbXBsZW1lbnRhdGlvbi4gSSBkb27igJl0IHRoaW5rIHdlIG5l ZWQgYW4gZXF1aXZhbGVudCBmb3IgZmxleGZpbGVzLg0KDQo+IA0KPiBUaGFua3MsDQo+IE5laWxC cm93bg0KPiANCj4gDQo+IE9uIEZyaSwgT2N0IDIxIDIwMTYsIE5laWxCcm93biB3cm90ZToNCj4g DQo+PiANCj4+IFZhcmlvdXMgcGxhY2VzIGFzc3VtZSB0aGF0IGlmIG5mczRfZmxfcHJlcGFyZV9k cygpIHR1cm5zIGEgbm9uLU5VTEwNCj4+ICdkcycsIHRoZW4gZHMtPmRzX2NscCB3aWxsIGFsc28g YmUgbm9uLU5VTEwuDQo+PiANCj4+IFRoaXMgaXMgbm90IG5lY2Vzc2FzcmlseSB0cnVlIGluIHRo ZSBjYXNlIHdoZW4gdGhlIHByb2Nlc3MgcmVjZWl2ZWQgYQ0KPj4gZmF0YWwgc2lnbmFsIHdoaWxl IG5mczRfcG5mc19kc19jb25uZWN0IGlzIHdhaXRpbmcgaW4NCj4+IG5mczRfd2FpdF9kc19jb25u ZWN0KCkuICBJbiB0aGF0IGNhc2UgLT5kc19jbHAgbWF5IG5vdCBiZSBzZXQsIGFuZCB0aGUNCj4+ IGRldmlkIG1heSBub3QgcmVjZW50bHkgaGF2ZSBiZWVuIG1hcmtlZCB1bmF2YWlsYWJsZS4NCj4+ IA0KPj4gU28gYWRkIGEgdGVzdCBmb3IgLT5kc19jbHAgPT0gTlVMTCBhbmQgcmV0dXJuIE5VTEwg aW4gdGhhdCBjYXNlLg0KPj4gDQo+PiBGaXhlczogYzIzMjY2ZDUzMmI0ICgiTkZTNC4xIEZpeCBk YXRhIHNlcnZlciBjb25uZWN0aW9uIHJhY2UiKQ0KPj4gU2lnbmVkLW9mZi1ieTogTmVpbEJyb3du IDxuZWlsYkBzdXNlLmNvbT4NCj4+IC0tLQ0KPj4gZnMvbmZzL2ZpbGVsYXlvdXQvZmlsZWxheW91 dGRldi5jIHwgMyArKy0NCj4+IDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDEgZGVs ZXRpb24oLSkNCj4+IA0KPj4gZGlmZiAtLWdpdCBhL2ZzL25mcy9maWxlbGF5b3V0L2ZpbGVsYXlv dXRkZXYuYyBiL2ZzL25mcy9maWxlbGF5b3V0L2ZpbGVsYXlvdXRkZXYuYw0KPj4gaW5kZXggNDk0 NmVmNDBiYTg3Li44NWVmMzhmOTc2NWYgMTAwNjQ0DQo+PiAtLS0gYS9mcy9uZnMvZmlsZWxheW91 dC9maWxlbGF5b3V0ZGV2LmMNCj4+ICsrKyBiL2ZzL25mcy9maWxlbGF5b3V0L2ZpbGVsYXlvdXRk ZXYuYw0KPj4gQEAgLTI4Myw3ICsyODMsOCBAQCBuZnM0X2ZsX3ByZXBhcmVfZHMoc3RydWN0IHBu ZnNfbGF5b3V0X3NlZ21lbnQgKmxzZWcsIHUzMiBkc19pZHgpDQo+PiAJCQkgICAgIHMtPm5mc19j bGllbnQtPmNsX3JwY2NsaWVudC0+Y2xfYXV0aC0+YXVfZmxhdm9yKTsNCj4+IA0KPj4gb3V0X3Rl c3RfZGV2aWQ6DQo+PiAtCWlmIChmaWxlbGF5b3V0X3Rlc3RfZGV2aWRfdW5hdmFpbGFibGUoZGV2 aWQpKQ0KPj4gKwlpZiAocmV0LT5kc19jbHAgPT0gTlVMTCB8fA0KPj4gKwkgICAgZmlsZWxheW91 dF90ZXN0X2RldmlkX3VuYXZhaWxhYmxlKGRldmlkKSkNCj4+IAkJcmV0ID0gTlVMTDsNCj4+IG91 dDoNCj4+IAlyZXR1cm4gcmV0Ow0KPj4gLS0gDQo+PiAyLjEwLjENCg0K -- 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 Fri, Nov 18, 2016 at 9:32 AM, Trond Myklebust <trondmy@primarydata.com> wrote: > >> On Nov 18, 2016, at 00:01, NeilBrown <neilb@suse.com> wrote: >> >> >> Ping? >> No sign of this in linux-next, and no replies…. > > I’d like some confirmation from the NetApp folks on this. They are the main stakeholders for the pNFS files implementation. I don’t think we need an equivalent for flex files. Just to see if I understand this patch. If "ds" isn't NULL, then the client has an RPC client with the ds. But it doesn't have a valid NFS client? Looking at the code I really don't see how that can happen. Maybe there is a bug elsewhere? If we return here, is there a chance we are going to have a zombie RPC client with the ds? If that's not a problem then I don't think there an issue to assume that if there is no valid NFS client then we would want to return a NULL from nfs4_fl_prepare_ds(). > >> >> Thanks, >> NeilBrown >> >> >> On Fri, Oct 21 2016, NeilBrown wrote: >> >>> >>> Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL >>> 'ds', then ds->ds_clp will also be non-NULL. >>> >>> This is not necessasrily true in the case when the process received a >>> fatal signal while nfs4_pnfs_ds_connect is waiting in >>> nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the >>> devid may not recently have been marked unavailable. >>> >>> So add a test for ->ds_clp == NULL and return NULL in that case. >>> >>> Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") >>> Signed-off-by: NeilBrown <neilb@suse.com> >>> --- >>> fs/nfs/filelayout/filelayoutdev.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c >>> index 4946ef40ba87..85ef38f9765f 100644 >>> --- a/fs/nfs/filelayout/filelayoutdev.c >>> +++ b/fs/nfs/filelayout/filelayoutdev.c >>> @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) >>> s->nfs_client->cl_rpcclient->cl_auth->au_flavor); >>> >>> out_test_devid: >>> - if (filelayout_test_devid_unavailable(devid)) >>> + if (ret->ds_clp == NULL || >>> + filelayout_test_devid_unavailable(devid)) >>> ret = NULL; >>> out: >>> return ret; >>> -- >>> 2.10.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
On Sat, Nov 19 2016, Olga Kornievskaia wrote: > On Fri, Nov 18, 2016 at 9:32 AM, Trond Myklebust > <trondmy@primarydata.com> wrote: >> >>> On Nov 18, 2016, at 00:01, NeilBrown <neilb@suse.com> wrote: >>> >>> >>> Ping? >>> No sign of this in linux-next, and no replies…. >> >> I’d like some confirmation from the NetApp folks on this. They are the main stakeholders for the pNFS files implementation. I don’t think we need an equivalent for flex files. > > Just to see if I understand this patch. If "ds" isn't NULL, then the > client has an RPC client with the ds. But it doesn't have a valid NFS > client? Looking at the code I really don't see how that can happen. I thought I explained that, but I was rather brief. nfs4_fl_prepare_ds() calls nfs4_pnfs_ds_connect() in order to create the NFS client. The first time it is called it will set NFS4DS_CONNECTING and then call _nfs4_pnfs_v?_ds_connect() which will establish the connection and set ds->ds_clp. If the server is unresponsive, this an block indefinitely. If a second thread then tries the same time, it will enter nfs4_pnfs_ds_connect() and will discovery that NFS4DS_CONNECTING is already set, so it will call nfs4_wait_ds_connect(). As nfs4_wait_ds_connect() waits with TASK_KILLABLE, it will abort if the process is kill by an uncaught signal (such as SIGKILL). In this case it will return even though NFS4DS_CONNECTING is still set, and ds->ds_clp is still NULL. i.e. the first thread hasn't established the connection yet. As the process has been killed, the 'ds' that it holds that doesn't have an NFS client handle will never be used. But we need to be sure that the process will exit cleanly without trying to de-reference that NULL ds_clp. That is what the patch ensures. > Maybe there is a bug elsewhere? If we return here, is there a chance > we are going to have a zombie RPC client with the ds? If that's not a > problem then I don't think there an issue to assume that if there is > no valid NFS client then we would want to return a NULL from > nfs4_fl_prepare_ds(). It isn't a "zombie RPC client", but rather an unborn RPC client, which still has NFS4DS_CONNECTING set. Thanks, NeilBrown > > >> >>> >>> Thanks, >>> NeilBrown >>> >>> >>> On Fri, Oct 21 2016, NeilBrown wrote: >>> >>>> >>>> Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL >>>> 'ds', then ds->ds_clp will also be non-NULL. >>>> >>>> This is not necessasrily true in the case when the process received a >>>> fatal signal while nfs4_pnfs_ds_connect is waiting in >>>> nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the >>>> devid may not recently have been marked unavailable. >>>> >>>> So add a test for ->ds_clp == NULL and return NULL in that case. >>>> >>>> Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") >>>> Signed-off-by: NeilBrown <neilb@suse.com> >>>> --- >>>> fs/nfs/filelayout/filelayoutdev.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c >>>> index 4946ef40ba87..85ef38f9765f 100644 >>>> --- a/fs/nfs/filelayout/filelayoutdev.c >>>> +++ b/fs/nfs/filelayout/filelayoutdev.c >>>> @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) >>>> s->nfs_client->cl_rpcclient->cl_auth->au_flavor); >>>> >>>> out_test_devid: >>>> - if (filelayout_test_devid_unavailable(devid)) >>>> + if (ret->ds_clp == NULL || >>>> + filelayout_test_devid_unavailable(devid)) >>>> ret = NULL; >>>> out: >>>> return ret; >>>> -- >>>> 2.10.1 >>
The patch looks good to me. We do need to ensure that ds_clp is set before returning a non-null nfs4_pnfs_ds from nfs4_fl_prepare_ds. As Trond pointed out, nfs4_ff_layout_prepare_ds already does this. →Andy On 11/19/16, 1:33 AM, "NeilBrown" <neilb@suse.com> wrote: On Sat, Nov 19 2016, Olga Kornievskaia wrote: > On Fri, Nov 18, 2016 at 9:32 AM, Trond Myklebust > <trondmy@primarydata.com> wrote: >> >>> On Nov 18, 2016, at 00:01, NeilBrown <neilb@suse.com> wrote: >>> >>> >>> Ping? >>> No sign of this in linux-next, and no replies…. >> >> I’d like some confirmation from the NetApp folks on this. They are the main stakeholders for the pNFS files implementation. I don’t think we need an equivalent for flex files. > > Just to see if I understand this patch. If "ds" isn't NULL, then the > client has an RPC client with the ds. But it doesn't have a valid NFS > client? Looking at the code I really don't see how that can happen. I thought I explained that, but I was rather brief. nfs4_fl_prepare_ds() calls nfs4_pnfs_ds_connect() in order to create the NFS client. The first time it is called it will set NFS4DS_CONNECTING and then call _nfs4_pnfs_v?_ds_connect() which will establish the connection and set ds->ds_clp. If the server is unresponsive, this an block indefinitely. If a second thread then tries the same time, it will enter nfs4_pnfs_ds_connect() and will discovery that NFS4DS_CONNECTING is already set, so it will call nfs4_wait_ds_connect(). As nfs4_wait_ds_connect() waits with TASK_KILLABLE, it will abort if the process is kill by an uncaught signal (such as SIGKILL). In this case it will return even though NFS4DS_CONNECTING is still set, and ds->ds_clp is still NULL. i.e. the first thread hasn't established the connection yet. As the process has been killed, the 'ds' that it holds that doesn't have an NFS client handle will never be used. But we need to be sure that the process will exit cleanly without trying to de-reference that NULL ds_clp. That is what the patch ensures. > Maybe there is a bug elsewhere? If we return here, is there a chance > we are going to have a zombie RPC client with the ds? If that's not a > problem then I don't think there an issue to assume that if there is > no valid NFS client then we would want to return a NULL from > nfs4_fl_prepare_ds(). It isn't a "zombie RPC client", but rather an unborn RPC client, which still has NFS4DS_CONNECTING set. Thanks, NeilBrown > > >> >>> >>> Thanks, >>> NeilBrown >>> >>> >>> On Fri, Oct 21 2016, NeilBrown wrote: >>> >>>> >>>> Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL >>>> 'ds', then ds->ds_clp will also be non-NULL. >>>> >>>> This is not necessasrily true in the case when the process received a >>>> fatal signal while nfs4_pnfs_ds_connect is waiting in >>>> nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the >>>> devid may not recently have been marked unavailable. >>>> >>>> So add a test for ->ds_clp == NULL and return NULL in that case. >>>> >>>> Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") >>>> Signed-off-by: NeilBrown <neilb@suse.com> >>>> --- >>>> fs/nfs/filelayout/filelayoutdev.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c >>>> index 4946ef40ba87..85ef38f9765f 100644 >>>> --- a/fs/nfs/filelayout/filelayoutdev.c >>>> +++ b/fs/nfs/filelayout/filelayoutdev.c >>>> @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) >>>> s->nfs_client->cl_rpcclient->cl_auth->au_flavor); >>>> >>>> out_test_devid: >>>> - if (filelayout_test_devid_unavailable(devid)) >>>> + if (ret->ds_clp == NULL || >>>> + filelayout_test_devid_unavailable(devid)) >>>> ret = NULL; >>>> out: >>>> return ret; >>>> -- >>>> 2.10.1 >>
On Sat, Nov 19, 2016 at 1:33 AM, NeilBrown <neilb@suse.com> wrote: > On Sat, Nov 19 2016, Olga Kornievskaia wrote: > >> On Fri, Nov 18, 2016 at 9:32 AM, Trond Myklebust >> <trondmy@primarydata.com> wrote: >>> >>>> On Nov 18, 2016, at 00:01, NeilBrown <neilb@suse.com> wrote: >>>> >>>> >>>> Ping? >>>> No sign of this in linux-next, and no replies…. >>> >>> I’d like some confirmation from the NetApp folks on this. They are the main stakeholders for the pNFS files implementation. I don’t think we need an equivalent for flex files. >> >> Just to see if I understand this patch. If "ds" isn't NULL, then the >> client has an RPC client with the ds. But it doesn't have a valid NFS >> client? Looking at the code I really don't see how that can happen. > > I thought I explained that, but I was rather brief. > > nfs4_fl_prepare_ds() calls nfs4_pnfs_ds_connect() in order to create the > NFS client. The first time it is called it will set NFS4DS_CONNECTING > and then call _nfs4_pnfs_v?_ds_connect() which will establish the > connection and set ds->ds_clp. If the server is unresponsive, this an > block indefinitely. > > If a second thread then tries the same time, it will enter > nfs4_pnfs_ds_connect() and will discovery that NFS4DS_CONNECTING is > already set, so it will call nfs4_wait_ds_connect(). > > As nfs4_wait_ds_connect() waits with TASK_KILLABLE, it will abort if the > process is kill by an uncaught signal (such as SIGKILL). > In this case it will return even though NFS4DS_CONNECTING is still set, > and ds->ds_clp is still NULL. i.e. the first thread hasn't established > the connection yet. > > As the process has been killed, the 'ds' that it holds that doesn't have > an NFS client handle will never be used. But we need to be sure that > the process will exit cleanly without trying to de-reference that NULL > ds_clp. > > That is what the patch ensures. Thanks for the explanation. Makes sense. >> Maybe there is a bug elsewhere? If we return here, is there a chance >> we are going to have a zombie RPC client with the ds? If that's not a >> problem then I don't think there an issue to assume that if there is >> no valid NFS client then we would want to return a NULL from >> nfs4_fl_prepare_ds(). > > It isn't a "zombie RPC client", but rather an unborn RPC client, which > still has NFS4DS_CONNECTING set. > > Thanks, > NeilBrown > > >> >> >>> >>>> >>>> Thanks, >>>> NeilBrown >>>> >>>> >>>> On Fri, Oct 21 2016, NeilBrown wrote: >>>> >>>>> >>>>> Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL >>>>> 'ds', then ds->ds_clp will also be non-NULL. >>>>> >>>>> This is not necessasrily true in the case when the process received a >>>>> fatal signal while nfs4_pnfs_ds_connect is waiting in >>>>> nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the >>>>> devid may not recently have been marked unavailable. >>>>> >>>>> So add a test for ->ds_clp == NULL and return NULL in that case. >>>>> >>>>> Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") >>>>> Signed-off-by: NeilBrown <neilb@suse.com> >>>>> --- >>>>> fs/nfs/filelayout/filelayoutdev.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c >>>>> index 4946ef40ba87..85ef38f9765f 100644 >>>>> --- a/fs/nfs/filelayout/filelayoutdev.c >>>>> +++ b/fs/nfs/filelayout/filelayoutdev.c >>>>> @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) >>>>> s->nfs_client->cl_rpcclient->cl_auth->au_flavor); >>>>> >>>>> out_test_devid: >>>>> - if (filelayout_test_devid_unavailable(devid)) >>>>> + if (ret->ds_clp == NULL || >>>>> + filelayout_test_devid_unavailable(devid)) >>>>> ret = NULL; >>>>> out: >>>>> return ret; >>>>> -- >>>>> 2.10.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
diff --git a/fs/nfs/filelayout/filelayoutdev.c b/fs/nfs/filelayout/filelayoutdev.c index 4946ef40ba87..85ef38f9765f 100644 --- a/fs/nfs/filelayout/filelayoutdev.c +++ b/fs/nfs/filelayout/filelayoutdev.c @@ -283,7 +283,8 @@ nfs4_fl_prepare_ds(struct pnfs_layout_segment *lseg, u32 ds_idx) s->nfs_client->cl_rpcclient->cl_auth->au_flavor); out_test_devid: - if (filelayout_test_devid_unavailable(devid)) + if (ret->ds_clp == NULL || + filelayout_test_devid_unavailable(devid)) ret = NULL; out: return ret;
Various places assume that if nfs4_fl_prepare_ds() turns a non-NULL 'ds', then ds->ds_clp will also be non-NULL. This is not necessasrily true in the case when the process received a fatal signal while nfs4_pnfs_ds_connect is waiting in nfs4_wait_ds_connect(). In that case ->ds_clp may not be set, and the devid may not recently have been marked unavailable. So add a test for ->ds_clp == NULL and return NULL in that case. Fixes: c23266d532b4 ("NFS4.1 Fix data server connection race") Signed-off-by: NeilBrown <neilb@suse.com> --- fs/nfs/filelayout/filelayoutdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)