From 37a217aca4a128d66a6dd4969375ea81bd879ac7 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Tue, 1 Sep 2020 19:20:37 -0700 Subject: Implement setattr+clunk in 9P This is to cover the common pattern: open->read/write->close, where SetAttr needs to be called to update atime/mtime before the file is closed. Benchmark results: BM_OpenReadClose/10240 CPU setattr+clunk: 63783 ns VFS2: 68109 ns VFS1: 72507 ns Updates #1198 PiperOrigin-RevId: 329628461 --- pkg/p9/client_file.go | 38 +++++++++- pkg/p9/file.go | 24 +++++++ pkg/p9/handlers.go | 31 +++++++++ pkg/p9/messages.go | 60 ++++++++++++++++ pkg/p9/messages_test.go | 24 +++++++ pkg/p9/p9.go | 162 ++++++++++++++++++++++--------------------- pkg/p9/p9test/client_test.go | 23 ++++-- pkg/p9/version.go | 8 ++- 8 files changed, 280 insertions(+), 90 deletions(-) (limited to 'pkg/p9') diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index 2ee07b664..28fe081d6 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -54,6 +54,8 @@ func (c *Client) newFile(fid FID) *clientFile { // // This proxies all of the interfaces found in file.go. type clientFile struct { + DisallowServerCalls + // client is the originating client. client *Client @@ -283,6 +285,39 @@ func (c *clientFile) Close() error { return nil } +// SetAttrClose implements File.SetAttrClose. +func (c *clientFile) SetAttrClose(valid SetAttrMask, attr SetAttr) error { + if !versionSupportsTsetattrclunk(c.client.version) { + setAttrErr := c.SetAttr(valid, attr) + + // Try to close file even in case of failure above. Since the state of the + // file is unknown to the caller, it will not attempt to close the file + // again. + if err := c.Close(); err != nil { + return err + } + + return setAttrErr + } + + // Avoid double close. + if !atomic.CompareAndSwapUint32(&c.closed, 0, 1) { + return syscall.EBADF + } + + // Send the message. + if err := c.client.sendRecv(&Tsetattrclunk{FID: c.fid, Valid: valid, SetAttr: attr}, &Rsetattrclunk{}); err != nil { + // If an error occurred, we toss away the FID. This isn't ideal, + // but I'm not sure what else makes sense in this context. + log.Warningf("Tsetattrclunk failed, losing FID %v: %v", c.fid, err) + return err + } + + // Return the FID to the pool. + c.client.fidPool.Put(uint64(c.fid)) + return nil +} + // Open implements File.Open. func (c *clientFile) Open(flags OpenFlags) (*fd.FD, QID, uint32, error) { if atomic.LoadUint32(&c.closed) != 0 { @@ -681,6 +716,3 @@ func (c *clientFile) Flush() error { return c.client.sendRecv(&Tflushf{FID: c.fid}, &Rflushf{}) } - -// Renamed implements File.Renamed. -func (c *clientFile) Renamed(newDir File, newName string) {} diff --git a/pkg/p9/file.go b/pkg/p9/file.go index cab35896f..c2e3a3f98 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -135,6 +135,14 @@ type File interface { // On the server, Close has no concurrency guarantee. Close() error + // SetAttrClose is the equivalent of calling SetAttr() followed by Close(). + // This can be used to set file times before closing the file in a single + // operation. + // + // On the server, SetAttr has a write concurrency guarantee. + // On the server, Close has no concurrency guarantee. + SetAttrClose(valid SetAttrMask, attr SetAttr) error + // Open must be called prior to using Read, Write or Readdir. Once Open // is called, some operations, such as Walk, will no longer work. // @@ -286,3 +294,19 @@ type DefaultWalkGetAttr struct{} func (DefaultWalkGetAttr) WalkGetAttr([]string) ([]QID, File, AttrMask, Attr, error) { return nil, nil, AttrMask{}, Attr{}, syscall.ENOSYS } + +// DisallowClientCalls panics if a client-only function is called. +type DisallowClientCalls struct{} + +// SetAttrClose implements File.SetAttrClose. +func (DisallowClientCalls) SetAttrClose(SetAttrMask, SetAttr) error { + panic("SetAttrClose should not be called on the server") +} + +// DisallowServerCalls panics if a server-only function is called. +type DisallowServerCalls struct{} + +// Renamed implements File.Renamed. +func (*clientFile) Renamed(File, string) { + panic("Renamed should not be called on the client") +} diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index 1db5797dd..abd237f46 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -123,6 +123,37 @@ func (t *Tclunk) handle(cs *connState) message { return &Rclunk{} } +func (t *Tsetattrclunk) handle(cs *connState) message { + ref, ok := cs.LookupFID(t.FID) + if !ok { + return newErr(syscall.EBADF) + } + defer ref.DecRef() + + setAttrErr := ref.safelyWrite(func() error { + // We don't allow setattr on files that have been deleted. + // This might be technically incorrect, as it's possible that + // there were multiple links and you can still change the + // corresponding inode information. + if ref.isDeleted() { + return syscall.EINVAL + } + + // Set the attributes. + return ref.file.SetAttr(t.Valid, t.SetAttr) + }) + + // Try to delete FID even in case of failure above. Since the state of the + // file is unknown to the caller, it will not attempt to close the file again. + if !cs.DeleteFID(t.FID) { + return newErr(syscall.EBADF) + } + if setAttrErr != nil { + return newErr(setAttrErr) + } + return &Rsetattrclunk{} +} + // handle implements handler.handle. func (t *Tremove) handle(cs *connState) message { ref, ok := cs.LookupFID(t.FID) diff --git a/pkg/p9/messages.go b/pkg/p9/messages.go index 2cb59f934..cf13cbb69 100644 --- a/pkg/p9/messages.go +++ b/pkg/p9/messages.go @@ -317,6 +317,64 @@ func (r *Rclunk) String() string { return "Rclunk{}" } +// Tsetattrclunk is a setattr+close request. +type Tsetattrclunk struct { + // FID is the FID to change. + FID FID + + // Valid is the set of bits which will be used. + Valid SetAttrMask + + // SetAttr is the set request. + SetAttr SetAttr +} + +// decode implements encoder.decode. +func (t *Tsetattrclunk) decode(b *buffer) { + t.FID = b.ReadFID() + t.Valid.decode(b) + t.SetAttr.decode(b) +} + +// encode implements encoder.encode. +func (t *Tsetattrclunk) encode(b *buffer) { + b.WriteFID(t.FID) + t.Valid.encode(b) + t.SetAttr.encode(b) +} + +// Type implements message.Type. +func (*Tsetattrclunk) Type() MsgType { + return MsgTsetattrclunk +} + +// String implements fmt.Stringer. +func (t *Tsetattrclunk) String() string { + return fmt.Sprintf("Tsetattrclunk{FID: %d, Valid: %v, SetAttr: %s}", t.FID, t.Valid, t.SetAttr) +} + +// Rsetattrclunk is a setattr+close response. +type Rsetattrclunk struct { +} + +// decode implements encoder.decode. +func (*Rsetattrclunk) decode(*buffer) { +} + +// encode implements encoder.encode. +func (*Rsetattrclunk) encode(*buffer) { +} + +// Type implements message.Type. +func (*Rsetattrclunk) Type() MsgType { + return MsgRsetattrclunk +} + +// String implements fmt.Stringer. +func (r *Rsetattrclunk) String() string { + return "Rsetattrclunk{}" +} + // Tremove is a remove request. // // This will eventually be replaced by Tunlinkat. @@ -2657,6 +2715,8 @@ func init() { msgRegistry.register(MsgRlconnect, func() message { return &Rlconnect{} }) msgRegistry.register(MsgTallocate, func() message { return &Tallocate{} }) msgRegistry.register(MsgRallocate, func() message { return &Rallocate{} }) + msgRegistry.register(MsgTsetattrclunk, func() message { return &Tsetattrclunk{} }) + msgRegistry.register(MsgRsetattrclunk, func() message { return &Rsetattrclunk{} }) msgRegistry.register(MsgTchannel, func() message { return &Tchannel{} }) msgRegistry.register(MsgRchannel, func() message { return &Rchannel{} }) } diff --git a/pkg/p9/messages_test.go b/pkg/p9/messages_test.go index 7facc9f5e..bfeb6c236 100644 --- a/pkg/p9/messages_test.go +++ b/pkg/p9/messages_test.go @@ -376,6 +376,30 @@ func TestEncodeDecode(t *testing.T) { &Rumknod{ Rmknod{QID: QID{Type: 1}}, }, + &Tsetattrclunk{ + FID: 1, + Valid: SetAttrMask{ + Permissions: true, + UID: true, + GID: true, + Size: true, + ATime: true, + MTime: true, + CTime: true, + ATimeNotSystemTime: true, + MTimeNotSystemTime: true, + }, + SetAttr: SetAttr{ + Permissions: 1, + UID: 2, + GID: 3, + Size: 4, + ATimeSeconds: 5, + ATimeNanoSeconds: 6, + MTimeSeconds: 7, + MTimeNanoSeconds: 8, + }, + }, } for _, enc := range objs { diff --git a/pkg/p9/p9.go b/pkg/p9/p9.go index 122c457d2..2235f8968 100644 --- a/pkg/p9/p9.go +++ b/pkg/p9/p9.go @@ -315,86 +315,88 @@ type MsgType uint8 // MsgType declarations. const ( - MsgTlerror MsgType = 6 - MsgRlerror = 7 - MsgTstatfs = 8 - MsgRstatfs = 9 - MsgTlopen = 12 - MsgRlopen = 13 - MsgTlcreate = 14 - MsgRlcreate = 15 - MsgTsymlink = 16 - MsgRsymlink = 17 - MsgTmknod = 18 - MsgRmknod = 19 - MsgTrename = 20 - MsgRrename = 21 - MsgTreadlink = 22 - MsgRreadlink = 23 - MsgTgetattr = 24 - MsgRgetattr = 25 - MsgTsetattr = 26 - MsgRsetattr = 27 - MsgTlistxattr = 28 - MsgRlistxattr = 29 - MsgTxattrwalk = 30 - MsgRxattrwalk = 31 - MsgTxattrcreate = 32 - MsgRxattrcreate = 33 - MsgTgetxattr = 34 - MsgRgetxattr = 35 - MsgTsetxattr = 36 - MsgRsetxattr = 37 - MsgTremovexattr = 38 - MsgRremovexattr = 39 - MsgTreaddir = 40 - MsgRreaddir = 41 - MsgTfsync = 50 - MsgRfsync = 51 - MsgTlink = 70 - MsgRlink = 71 - MsgTmkdir = 72 - MsgRmkdir = 73 - MsgTrenameat = 74 - MsgRrenameat = 75 - MsgTunlinkat = 76 - MsgRunlinkat = 77 - MsgTversion = 100 - MsgRversion = 101 - MsgTauth = 102 - MsgRauth = 103 - MsgTattach = 104 - MsgRattach = 105 - MsgTflush = 108 - MsgRflush = 109 - MsgTwalk = 110 - MsgRwalk = 111 - MsgTread = 116 - MsgRread = 117 - MsgTwrite = 118 - MsgRwrite = 119 - MsgTclunk = 120 - MsgRclunk = 121 - MsgTremove = 122 - MsgRremove = 123 - MsgTflushf = 124 - MsgRflushf = 125 - MsgTwalkgetattr = 126 - MsgRwalkgetattr = 127 - MsgTucreate = 128 - MsgRucreate = 129 - MsgTumkdir = 130 - MsgRumkdir = 131 - MsgTumknod = 132 - MsgRumknod = 133 - MsgTusymlink = 134 - MsgRusymlink = 135 - MsgTlconnect = 136 - MsgRlconnect = 137 - MsgTallocate = 138 - MsgRallocate = 139 - MsgTchannel = 250 - MsgRchannel = 251 + MsgTlerror MsgType = 6 + MsgRlerror MsgType = 7 + MsgTstatfs MsgType = 8 + MsgRstatfs MsgType = 9 + MsgTlopen MsgType = 12 + MsgRlopen MsgType = 13 + MsgTlcreate MsgType = 14 + MsgRlcreate MsgType = 15 + MsgTsymlink MsgType = 16 + MsgRsymlink MsgType = 17 + MsgTmknod MsgType = 18 + MsgRmknod MsgType = 19 + MsgTrename MsgType = 20 + MsgRrename MsgType = 21 + MsgTreadlink MsgType = 22 + MsgRreadlink MsgType = 23 + MsgTgetattr MsgType = 24 + MsgRgetattr MsgType = 25 + MsgTsetattr MsgType = 26 + MsgRsetattr MsgType = 27 + MsgTlistxattr MsgType = 28 + MsgRlistxattr MsgType = 29 + MsgTxattrwalk MsgType = 30 + MsgRxattrwalk MsgType = 31 + MsgTxattrcreate MsgType = 32 + MsgRxattrcreate MsgType = 33 + MsgTgetxattr MsgType = 34 + MsgRgetxattr MsgType = 35 + MsgTsetxattr MsgType = 36 + MsgRsetxattr MsgType = 37 + MsgTremovexattr MsgType = 38 + MsgRremovexattr MsgType = 39 + MsgTreaddir MsgType = 40 + MsgRreaddir MsgType = 41 + MsgTfsync MsgType = 50 + MsgRfsync MsgType = 51 + MsgTlink MsgType = 70 + MsgRlink MsgType = 71 + MsgTmkdir MsgType = 72 + MsgRmkdir MsgType = 73 + MsgTrenameat MsgType = 74 + MsgRrenameat MsgType = 75 + MsgTunlinkat MsgType = 76 + MsgRunlinkat MsgType = 77 + MsgTversion MsgType = 100 + MsgRversion MsgType = 101 + MsgTauth MsgType = 102 + MsgRauth MsgType = 103 + MsgTattach MsgType = 104 + MsgRattach MsgType = 105 + MsgTflush MsgType = 108 + MsgRflush MsgType = 109 + MsgTwalk MsgType = 110 + MsgRwalk MsgType = 111 + MsgTread MsgType = 116 + MsgRread MsgType = 117 + MsgTwrite MsgType = 118 + MsgRwrite MsgType = 119 + MsgTclunk MsgType = 120 + MsgRclunk MsgType = 121 + MsgTremove MsgType = 122 + MsgRremove MsgType = 123 + MsgTflushf MsgType = 124 + MsgRflushf MsgType = 125 + MsgTwalkgetattr MsgType = 126 + MsgRwalkgetattr MsgType = 127 + MsgTucreate MsgType = 128 + MsgRucreate MsgType = 129 + MsgTumkdir MsgType = 130 + MsgRumkdir MsgType = 131 + MsgTumknod MsgType = 132 + MsgRumknod MsgType = 133 + MsgTusymlink MsgType = 134 + MsgRusymlink MsgType = 135 + MsgTlconnect MsgType = 136 + MsgRlconnect MsgType = 137 + MsgTallocate MsgType = 138 + MsgRallocate MsgType = 139 + MsgTsetattrclunk MsgType = 140 + MsgRsetattrclunk MsgType = 141 + MsgTchannel MsgType = 250 + MsgRchannel MsgType = 251 ) // QIDType represents the file type for QIDs. diff --git a/pkg/p9/p9test/client_test.go b/pkg/p9/p9test/client_test.go index 6e7bb3db2..6e605b14c 100644 --- a/pkg/p9/p9test/client_test.go +++ b/pkg/p9/p9test/client_test.go @@ -1225,22 +1225,31 @@ func TestOpen(t *testing.T) { func TestClose(t *testing.T) { type closeTest struct { name string - closeFn func(backend *Mock, f p9.File) + closeFn func(backend *Mock, f p9.File) error } cases := []closeTest{ { name: "close", - closeFn: func(_ *Mock, f p9.File) { - f.Close() + closeFn: func(_ *Mock, f p9.File) error { + return f.Close() }, }, { name: "remove", - closeFn: func(backend *Mock, f p9.File) { + closeFn: func(backend *Mock, f p9.File) error { // Allow the rename call in the parent, automatically translated. backend.parent.EXPECT().UnlinkAt(gomock.Any(), gomock.Any()).Times(1) - f.(deprecatedRemover).Remove() + return f.(deprecatedRemover).Remove() + }, + }, + { + name: "setAttrClose", + closeFn: func(backend *Mock, f p9.File) error { + valid := p9.SetAttrMask{ATime: true} + attr := p9.SetAttr{ATimeSeconds: 1, ATimeNanoSeconds: 2} + backend.EXPECT().SetAttr(valid, attr).Times(1) + return f.SetAttrClose(valid, attr) }, }, } @@ -1258,7 +1267,9 @@ func TestClose(t *testing.T) { _, backend, f := walkHelper(h, name, root) // Close via the prescribed method. - tc.closeFn(backend, f) + if err := tc.closeFn(backend, f); err != nil { + t.Fatalf("closeFn failed: %v", err) + } // Everything should fail with EBADF. if _, _, err := f.Walk(nil); err != syscall.EBADF { diff --git a/pkg/p9/version.go b/pkg/p9/version.go index 09cde9f5a..8d7168ef5 100644 --- a/pkg/p9/version.go +++ b/pkg/p9/version.go @@ -26,7 +26,7 @@ const ( // // Clients are expected to start requesting this version number and // to continuously decrement it until a Tversion request succeeds. - highestSupportedVersion uint32 = 11 + highestSupportedVersion uint32 = 12 // lowestSupportedVersion is the lowest supported version X in a // version string of the format 9P2000.L.Google.X. @@ -173,3 +173,9 @@ func versionSupportsGetSetXattr(v uint32) bool { func versionSupportsListRemoveXattr(v uint32) bool { return v >= 11 } + +// versionSupportsTsetattrclunk returns true if version v supports +// the Tsetattrclunk message. +func versionSupportsTsetattrclunk(v uint32) bool { + return v >= 12 +} -- cgit v1.2.3