[04/19] pnfs: Add layout driver flag PNFS_LAYOUTGET_ON_OPEN
diff mbox

Message ID 20180530180553.38769-5-trond.myklebust@hammerspace.com
State New
Headers show

Commit Message

Trond Myklebust May 30, 2018, 6:05 p.m. UTC
From: Fred Isaman <fred.isaman@gmail.com>

Driver can set flag to allow LAYOUTGET to be sent with OPEN.

Signed-off-by: Fred Isaman <fred.isaman@gmail.com>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/flexfilelayout/flexfilelayout.c | 1 +
 fs/nfs/pnfs.h                          | 1 +
 2 files changed, 2 insertions(+)

Comments

Olga Kornievskaia May 30, 2018, 8:10 p.m. UTC | #1
Hi Trond,

This feature is enabled for Flexfiles layout type. Is there a reason
that it shouldn't be generic for all pnfs?

On Wed, May 30, 2018 at 2:05 PM, Trond Myklebust <trondmy@gmail.com> wrote:
> From: Fred Isaman <fred.isaman@gmail.com>
>
> Driver can set flag to allow LAYOUTGET to be sent with OPEN.
>
> Signed-off-by: Fred Isaman <fred.isaman@gmail.com>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/flexfilelayout/flexfilelayout.c | 1 +
>  fs/nfs/pnfs.h                          | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> index c75ad982bcfc..3ae038d9c292 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> @@ -2347,6 +2347,7 @@ static struct pnfs_layoutdriver_type flexfilelayout_type = {
>         .id                     = LAYOUT_FLEX_FILES,
>         .name                   = "LAYOUT_FLEX_FILES",
>         .owner                  = THIS_MODULE,
> +       .flags                  = PNFS_LAYOUTGET_ON_OPEN,
>         .set_layoutdriver       = ff_layout_set_layoutdriver,
>         .alloc_layout_hdr       = ff_layout_alloc_layout_hdr,
>         .free_layout_hdr        = ff_layout_free_layout_hdr,
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index daf6cbf5c15f..f71a55f11b97 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -110,6 +110,7 @@ enum layoutdriver_policy_flags {
>         PNFS_LAYOUTRET_ON_SETATTR       = 1 << 0,
>         PNFS_LAYOUTRET_ON_ERROR         = 1 << 1,
>         PNFS_READ_WHOLE_PAGE            = 1 << 2,
> +       PNFS_LAYOUTGET_ON_OPEN          = 1 << 3,
>  };
>
>  struct nfs4_deviceid_node;
> --
> 2.17.0
>
> --
> 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
--
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
Trond Myklebust May 31, 2018, 12:40 p.m. UTC | #2
T24gV2VkLCAyMDE4LTA1LTMwIGF0IDE2OjEwIC0wNDAwLCBPbGdhIEtvcm5pZXZza2FpYSB3cm90
ZToNCj4gSGkgVHJvbmQsDQo+IA0KPiBUaGlzIGZlYXR1cmUgaXMgZW5hYmxlZCBmb3IgRmxleGZp
bGVzIGxheW91dCB0eXBlLiBJcyB0aGVyZSBhIHJlYXNvbg0KPiB0aGF0IGl0IHNob3VsZG4ndCBi
ZSBnZW5lcmljIGZvciBhbGwgcG5mcz8NCg0KWWVzIGFuZCBuby4gSXQgcmVsaWVzIG9uIHVzZSBv
ZiB0aGUgImN1cnJlbnQgc3RhdGVpZCIgZmVhdHVyZSwgd2hpY2gNCmhhcyBub3QgcHJldmlvdXNs
eSBzZWVuIGhlYXZ5IHVzZSwgYW5kIHRoZSBhYmlsaXR5IG9mIHRoZSBwTkZTIHNlcnZlcg0KdG8g
aGFuZGxlIGxheW91dGdldCByYWNlcyBjb3JyZWN0bHkgKHdoZW4gdGhlIGNsaWVudCBzZW5kcyAy
IGxheW91dGdldA0KcmVxdWVzdHMgaW4gcGFyYWxsZWwsIGJvdGggdXNpbmcgdGhlIG9wZW4gc3Rh
dGVpZCkuIElmIHRoZSBzZXJ2ZXIgaXMNCmludmFsaWRhdGluZyBvbmUgb2YgdGhlIHJlc3VsdGlu
ZyBsYXlvdXQgc3RhdGVpZHMgYXMgaXQgc2hvdWxkLCB0aGVuIGl0DQpoYWQgYmV0dGVyIGVpdGhl
ciBoYXZlIGEgZ29vZCBmZW5jaW5nIG1lY2hhbmlzbSwgb3IgaXQgbXVzdCByZWNhbGwgdGhhdA0K
bGF5b3V0IHN0YXRlaWQgYmVmb3JlIGhhbmRpbmcgb3V0IHRoZSBzZWNvbmQgbGF5b3V0Lg0KDQpG
b3IgdGhhdCByZWFzb24sIEknZCBsaWtlIHRvIGVuc3VyZSB0aGF0IHdlIGF0IGxlYXN0IHRlc3Qg
dGhlIGV4aXN0aW5nDQpwTkZTIGltcGxlbWVudGF0aW9ucyB0byBlbnN1cmUgd2UgZG9uJ3Qgc2Vl
IHJlZ3Jlc3Npb25zIGJlZm9yZSB3ZQ0KZW5hYmxlIHRoZSBmZWF0dXJlLg0KDQoNCj4gT24gV2Vk
LCBNYXkgMzAsIDIwMTggYXQgMjowNSBQTSwgVHJvbmQgTXlrbGVidXN0IDx0cm9uZG15QGdtYWls
LmNvbT4NCj4gd3JvdGU6DQo+ID4gRnJvbTogRnJlZCBJc2FtYW4gPGZyZWQuaXNhbWFuQGdtYWls
LmNvbT4NCj4gPiANCj4gPiBEcml2ZXIgY2FuIHNldCBmbGFnIHRvIGFsbG93IExBWU9VVEdFVCB0
byBiZSBzZW50IHdpdGggT1BFTi4NCj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5OiBGcmVkIElzYW1h
biA8ZnJlZC5pc2FtYW5AZ21haWwuY29tPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xl
YnVzdCA8dHJvbmQubXlrbGVidXN0QGhhbW1lcnNwYWNlLmNvbT4NCj4gPiAtLS0NCj4gPiAgZnMv
bmZzL2ZsZXhmaWxlbGF5b3V0L2ZsZXhmaWxlbGF5b3V0LmMgfCAxICsNCj4gPiAgZnMvbmZzL3Bu
ZnMuaCAgICAgICAgICAgICAgICAgICAgICAgICAgfCAxICsNCj4gPiAgMiBmaWxlcyBjaGFuZ2Vk
LCAyIGluc2VydGlvbnMoKykNCj4gPiANCj4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL2ZsZXhmaWxl
bGF5b3V0L2ZsZXhmaWxlbGF5b3V0LmMNCj4gPiBiL2ZzL25mcy9mbGV4ZmlsZWxheW91dC9mbGV4
ZmlsZWxheW91dC5jDQo+ID4gaW5kZXggYzc1YWQ5ODJiY2ZjLi4zYWUwMzhkOWMyOTIgMTAwNjQ0
DQo+ID4gLS0tIGEvZnMvbmZzL2ZsZXhmaWxlbGF5b3V0L2ZsZXhmaWxlbGF5b3V0LmMNCj4gPiAr
KysgYi9mcy9uZnMvZmxleGZpbGVsYXlvdXQvZmxleGZpbGVsYXlvdXQuYw0KPiA+IEBAIC0yMzQ3
LDYgKzIzNDcsNyBAQCBzdGF0aWMgc3RydWN0IHBuZnNfbGF5b3V0ZHJpdmVyX3R5cGUNCj4gPiBm
bGV4ZmlsZWxheW91dF90eXBlID0gew0KPiA+ICAgICAgICAgLmlkICAgICAgICAgICAgICAgICAg
ICAgPSBMQVlPVVRfRkxFWF9GSUxFUywNCj4gPiAgICAgICAgIC5uYW1lICAgICAgICAgICAgICAg
ICAgID0gIkxBWU9VVF9GTEVYX0ZJTEVTIiwNCj4gPiAgICAgICAgIC5vd25lciAgICAgICAgICAg
ICAgICAgID0gVEhJU19NT0RVTEUsDQo+ID4gKyAgICAgICAuZmxhZ3MgICAgICAgICAgICAgICAg
ICA9IFBORlNfTEFZT1VUR0VUX09OX09QRU4sDQo+ID4gICAgICAgICAuc2V0X2xheW91dGRyaXZl
ciAgICAgICA9IGZmX2xheW91dF9zZXRfbGF5b3V0ZHJpdmVyLA0KPiA+ICAgICAgICAgLmFsbG9j
X2xheW91dF9oZHIgICAgICAgPSBmZl9sYXlvdXRfYWxsb2NfbGF5b3V0X2hkciwNCj4gPiAgICAg
ICAgIC5mcmVlX2xheW91dF9oZHIgICAgICAgID0gZmZfbGF5b3V0X2ZyZWVfbGF5b3V0X2hkciwN
Cj4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL3BuZnMuaCBiL2ZzL25mcy9wbmZzLmgNCj4gPiBpbmRl
eCBkYWY2Y2JmNWMxNWYuLmY3MWE1NWYxMWI5NyAxMDA2NDQNCj4gPiAtLS0gYS9mcy9uZnMvcG5m
cy5oDQo+ID4gKysrIGIvZnMvbmZzL3BuZnMuaA0KPiA+IEBAIC0xMTAsNiArMTEwLDcgQEAgZW51
bSBsYXlvdXRkcml2ZXJfcG9saWN5X2ZsYWdzIHsNCj4gPiAgICAgICAgIFBORlNfTEFZT1VUUkVU
X09OX1NFVEFUVFIgICAgICAgPSAxIDw8IDAsDQo+ID4gICAgICAgICBQTkZTX0xBWU9VVFJFVF9P
Tl9FUlJPUiAgICAgICAgID0gMSA8PCAxLA0KPiA+ICAgICAgICAgUE5GU19SRUFEX1dIT0xFX1BB
R0UgICAgICAgICAgICA9IDEgPDwgMiwNCj4gPiArICAgICAgIFBORlNfTEFZT1VUR0VUX09OX09Q
RU4gICAgICAgICAgPSAxIDw8IDMsDQo+ID4gIH07DQo+ID4gDQo+ID4gIHN0cnVjdCBuZnM0X2Rl
dmljZWlkX25vZGU7DQo+ID4gLS0NCj4gPiAyLjE3LjANCj4gPiANCj4gPiAtLQ0KPiA+IFRvIHVu
c3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51
eC0NCj4gPiBuZnMiIGluDQo+ID4gdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2
Z2VyLmtlcm5lbC5vcmcNCj4gPiBNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5r
ZXJuZWwub3JnL21ham9yZG9tby1pbmZvLmh0bWwNCj4gDQo+IC0tDQo+IFRvIHVuc3Vic2NyaWJl
IGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1uZnMiDQo+
IGluDQo+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3Jn
DQo+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jk
b21vLWluZm8uaHRtbA0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFp
bnRhaW5lciwgSGFtbWVyc3BhY2UNCnRyb25kLm15a2xlYnVzdEBoYW1tZXJzcGFjZS5jb20NCg0K
--
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
Olga Kornievskaia Sept. 6, 2019, 8:17 p.m. UTC | #3
On Thu, May 31, 2018 at 8:41 AM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Wed, 2018-05-30 at 16:10 -0400, Olga Kornievskaia wrote:
> > Hi Trond,
> >
> > This feature is enabled for Flexfiles layout type. Is there a reason
> > that it shouldn't be generic for all pnfs?
>
> Yes and no. It relies on use of the "current stateid" feature, which
> has not previously seen heavy use, and the ability of the pNFS server
> to handle layoutget races correctly (when the client sends 2 layoutget
> requests in parallel, both using the open stateid). If the server is
> invalidating one of the resulting layout stateids as it should, then it
> had better either have a good fencing mechanism, or it must recall that
> layout stateid before handing out the second layout.
>
> For that reason, I'd like to ensure that we at least test the existing
> pNFS implementations to ensure we don't see regressions before we
> enable the feature.

Hi Trond,

I'm getting back to trying to add LAYOUTGET to the OPEN compound for
the file layout type. I'm looking at your reply and trying to figure
out what kind of testing I should try to do. You mention a race where
a client sends 2 layoutget requests in parallel. You say "using the
open stateid", but when the LAYOUTGET is added to the OPEN, it'll be
using as you said current stateid. So is the problem sending 2
concurrent OPENs for the same file (same owner)? Then server should
issue the reply to the 1st, then a layout recall and reply to the 2nd?

But for the testing is your suggestion to instrument sending 2
concurrent opens with the new code and see that happens?

Thank you.

> > On Wed, May 30, 2018 at 2:05 PM, Trond Myklebust <trondmy@gmail.com>
> > wrote:
> > > From: Fred Isaman <fred.isaman@gmail.com>
> > >
> > > Driver can set flag to allow LAYOUTGET to be sent with OPEN.
> > >
> > > Signed-off-by: Fred Isaman <fred.isaman@gmail.com>
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/flexfilelayout/flexfilelayout.c | 1 +
> > >  fs/nfs/pnfs.h                          | 1 +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c
> > > b/fs/nfs/flexfilelayout/flexfilelayout.c
> > > index c75ad982bcfc..3ae038d9c292 100644
> > > --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> > > +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> > > @@ -2347,6 +2347,7 @@ static struct pnfs_layoutdriver_type
> > > flexfilelayout_type = {
> > >         .id                     = LAYOUT_FLEX_FILES,
> > >         .name                   = "LAYOUT_FLEX_FILES",
> > >         .owner                  = THIS_MODULE,
> > > +       .flags                  = PNFS_LAYOUTGET_ON_OPEN,
> > >         .set_layoutdriver       = ff_layout_set_layoutdriver,
> > >         .alloc_layout_hdr       = ff_layout_alloc_layout_hdr,
> > >         .free_layout_hdr        = ff_layout_free_layout_hdr,
> > > diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> > > index daf6cbf5c15f..f71a55f11b97 100644
> > > --- a/fs/nfs/pnfs.h
> > > +++ b/fs/nfs/pnfs.h
> > > @@ -110,6 +110,7 @@ enum layoutdriver_policy_flags {
> > >         PNFS_LAYOUTRET_ON_SETATTR       = 1 << 0,
> > >         PNFS_LAYOUTRET_ON_ERROR         = 1 << 1,
> > >         PNFS_READ_WHOLE_PAGE            = 1 << 2,
> > > +       PNFS_LAYOUTGET_ON_OPEN          = 1 << 3,
> > >  };
> > >
> > >  struct nfs4_deviceid_node;
> > > --
> > > 2.17.0
> > >
> > > --
> > > 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
> >
> > --
> > 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
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>

Patch
diff mbox

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index c75ad982bcfc..3ae038d9c292 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -2347,6 +2347,7 @@  static struct pnfs_layoutdriver_type flexfilelayout_type = {
 	.id			= LAYOUT_FLEX_FILES,
 	.name			= "LAYOUT_FLEX_FILES",
 	.owner			= THIS_MODULE,
+	.flags			= PNFS_LAYOUTGET_ON_OPEN,
 	.set_layoutdriver	= ff_layout_set_layoutdriver,
 	.alloc_layout_hdr	= ff_layout_alloc_layout_hdr,
 	.free_layout_hdr	= ff_layout_free_layout_hdr,
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index daf6cbf5c15f..f71a55f11b97 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -110,6 +110,7 @@  enum layoutdriver_policy_flags {
 	PNFS_LAYOUTRET_ON_SETATTR	= 1 << 0,
 	PNFS_LAYOUTRET_ON_ERROR		= 1 << 1,
 	PNFS_READ_WHOLE_PAGE		= 1 << 2,
+	PNFS_LAYOUTGET_ON_OPEN		= 1 << 3,
 };
 
 struct nfs4_deviceid_node;