Handling INTERRUPT requests about go-fuse HOT 7 CLOSED

2 remarks:

There is already a lock for request management in Server (reqMu).

Can you see if you put your inflight data protected by the same lock?

Also, I wonder if we can avoid the requestID map[]. You have to 2 map
lookups for every request, and most of them are unnecessary as interrupts
happen rarely.

How about having a slice of outstanding requests, and in each request,
annotate the request with the index in the slice. Then, for interrupt, do
a linear search. This puts the cost of interrupt in the interrupt
processing itself rather than in the normal path.

On Thu, Jun 27, 2013 at 12:11 PM, Alessandro Arzilli <
[email protected]> wrote:

I see that go-fuse ignores INTERRUPT requests from fuse. At least on linux
this means that signal handling in the client is delayed indefinitely until
the original interrupted request returns. It's not a bug but it's jarring
and I'd like to see INTERRUPT requests handled.

I have written a partial patch for one possible implementation that only
covers nodefs filesystem. In this implementation the fuse.Context object
contains a channel named Interrupted a true value will be written to this
channel for an interrupted request.

Author: aarzilli [email protected]
Date: Thu Jun 27 11:52:59 2013 +0200

INTERRUPT handling

diff --git a/fuse/nodefs/api.go b/fuse/nodefs/api.go
index 36414c2..94d241a 100644
--- a/fuse/nodefs/api.go
+++ b/fuse/nodefs/api.go
@@ -112,8 +112,8 @@ type File interface {
// the inner file here.
InnerFile() File

• Read(dest []byte, off int64) (fuse.ReadResult, fuse.Status)
• Write(data []byte, off int64) (written uint32, code fuse.Status)
• Read(dest []byte, off int64, context *fuse.Context) (fuse.ReadResult, fuse.Status)
• Write(data []byte, off int64, context *fuse.Context) (written uint32, code fuse.Status)
  Flush() fuse.Status
  Release()
  Fsync(flags int) (code fuse.Status)
  diff --git a/fuse/nodefs/defaultfile.go b/fuse/nodefs/defaultfile.go
  index 27df378..87889e4 100644
  --- a/fuse/nodefs/defaultfile.go
  +++ b/fuse/nodefs/defaultfile.go
  @@ -26,11 +26,11 @@ func (f *defaultFile) String() string {
  return "defaultFile"
  }

-func (f *defaultFile) Read(buf []byte, off int64) (fuse.ReadResult, fuse.Status) {
+func (f *defaultFile) Read(buf []byte, off int64, context *fuse.Context) (fuse.ReadResult, fuse.Status) {
return nil, fuse.ENOSYS
}

-func (f *defaultFile) Write(data []byte, off int64) (uint32, fuse.Status) {
+func (f *defaultFile) Write(data []byte, off int64, context *fuse.Context) (uint32, fuse.Status) {
return 0, fuse.ENOSYS
}

diff --git a/fuse/nodefs/files.go b/fuse/nodefs/files.go
index e00d54d..136515a 100644
--- a/fuse/nodefs/files.go
+++ b/fuse/nodefs/files.go
@@ -43,7 +43,7 @@ func NewDataFile(data []byte) File {
return f
}

-func (f *dataFile) Read(buf []byte, off int64) (res fuse.ReadResult, code fuse.Status) {
+func (f *dataFile) Read(buf []byte, off int64, context *fuse.Context) (res fuse.ReadResult, code fuse.Status) {
end := int(off) + int(len(buf))
if end > len(f.data) {
end = len(f.data)
@@ -74,11 +74,11 @@ func (f *devNullFile) String() string {
return "devNullFile"
}

-func (f *devNullFile) Read(buf []byte, off int64) (fuse.ReadResult, fuse.Status) {
+func (f *devNullFile) Read(buf []byte, off int64, context *fuse.Context) (fuse.ReadResult, fuse.Status) {
return &fuse.ReadResultData{}, fuse.OK
}

-func (f *devNullFile) Write(content []byte, off int64) (uint32, fuse.Status) {
+func (f *devNullFile) Write(content []byte, off int64, context *fuse.Context) (uint32, fuse.Status) {
return uint32(len(content)), fuse.OK
}

@@ -125,7 +125,7 @@ func (f *loopbackFile) String() string {
return fmt.Sprintf("loopbackFile(%s)", f.File.Name())
}

-func (f *loopbackFile) Read(buf []byte, off int64) (res fuse.ReadResult, code fuse.Status) {
+func (f *loopbackFile) Read(buf []byte, off int64, context *fuse.Context) (res fuse.ReadResult, code fuse.Status) {
f.lock.Lock()
r := &fuse.ReadResultFd{
Fd: f.File.Fd(),
@@ -136,7 +136,7 @@ func (f *loopbackFile) Read(buf []byte, off int64) (res fuse.ReadResult, code fu
return r, fuse.OK
}

-func (f *loopbackFile) Write(data []byte, off int64) (uint32, fuse.Status) {
+func (f *loopbackFile) Write(data []byte, off int64, context *fuse.Context) (uint32, fuse.Status) {
f.lock.Lock()
n, err := f.File.WriteAt(data, off)
f.lock.Unlock()
@@ -229,7 +229,7 @@ func (f *readOnlyFile) String() string {
return fmt.Sprintf("readOnlyFile(%s)", f.File.String())
}

-func (f *readOnlyFile) Write(data []byte, off int64) (uint32, fuse.Status) {
+func (f *readOnlyFile) Write(data []byte, off int64, context *fuse.Context) (uint32, fuse.Status) {
return 0, fuse.EPERM
}

diff --git a/fuse/nodefs/fsops.go b/fuse/nodefs/fsops.go
index 31e51c5..65ec578 100644
--- a/fuse/nodefs/fsops.go
+++ b/fuse/nodefs/fsops.go
@@ -403,14 +403,14 @@ func (c *rawBridge) ListXAttr(context *fuse.Context) (data []byte, code fuse.Sta
func (c *rawBridge) Write(context *fuse.Context, input *raw.WriteIn, data []byte) (written uint32, code fuse.Status) {
node := c.toInode(context.NodeId)
opened := node.mount.getOpenedFile(input.Fh)

• return opened.WithFlags.File.Write(data, int64(input.Offset))
• return opened.WithFlags.File.Write(data, int64(input.Offset), context)
  }

func (c *rawBridge) Read(context *fuse.Context, input *raw.ReadIn, buf []byte) (fuse.ReadResult, fuse.Status) {
node := c.toInode(context.NodeId)
opened := node.mount.getOpenedFile(input.Fh)

• return opened.WithFlags.File.Read(buf, int64(input.Offset))
• return opened.WithFlags.File.Read(buf, int64(input.Offset), context)
  }

func (c *rawBridge) StatFs(out *raw.StatfsOut, context *fuse.Context) fuse.Status {
diff --git a/fuse/request.go b/fuse/request.go
index 37a2859..c071bc1 100644
--- a/fuse/request.go
+++ b/fuse/request.go
@@ -70,6 +70,7 @@ func (r *request) clear() {
r.startTime = time.Time{}
r.handler = nil
r.readResult = nil

• r.context.Interrupted = make(chan bool, 1)
  }

func (r *request) InputDebug() string {
diff --git a/fuse/server.go b/fuse/server.go
index 7f1696b..ed1ed2a 100644
--- a/fuse/server.go
+++ b/fuse/server.go
@@ -45,6 +45,9 @@ type Server struct {
outstandingReadBufs int
kernelSettings raw.InitIn

• flightMu sync.Mutex
• inFlight map[uint64]_request

canSplice bool
loops sync.WaitGroup
}
@@ -146,8 +149,9 @@ func NewServer(fs RawFileSystem, mountPoint string, opts *MountOptions) (_Server
fileSystem: fs,
started: make(chan struct{}),
opts: &o,

•   inFlight: make(map[uint64]*request),
  

  }

  optStrs := opts.Options
  if opts.AllowOther {
  optStrs = append(optStrs, "allow_other")
  @@ -225,6 +229,7 @@ func (ms *Server) readRequest(exitIdle bool) (req *request, code Status) {
  ms.reqPool = ms.reqPool[:l-1]
  } else {
  req = new(request)

•   req.context.Interrupted = make(chan bool, 1)
  

  }
  l = len(ms.readPool)
  if l > 0 {
  @@ -334,6 +339,24 @@ exit:
  }
  }

+func (ms *Server) getInFlight(unique uint64) *request {

• ms.flightMu.Lock()
• defer ms.flightMu.Unlock()
• return ms.inFlight[unique]
  +}

+func (ms *Server) pushInFlight(req *request) {

• ms.flightMu.Lock()
• defer ms.flightMu.Unlock()
• ms.inFlight[req.inHeader.Unique] = req
  +}

+func (ms *Server) popInFlight(req *request) {

• ms.flightMu.Lock()
• defer ms.flightMu.Unlock()
• delete(ms.inFlight, req.inHeader.Unique)
  +}

func (ms *Server) handleRequest(req *request) {
req.parse()
if req.handler == nil {
@@ -344,21 +367,42 @@ func (ms *Server) handleRequest(req *request) {
log.Println(req.InputDebug())
}

• if req.status.Ok() && req.handler.Func == nil {

•   log.Printf("Unimplemented opcode %v", operationName(req.inHeader.Opcode))
  

•   req.status = ENOSYS
  

• }
• if req.status.Ok() && (req.inHeader.Opcode == _OP_INTERRUPT) {

•   interreq := ms.getInFlight((*raw.InterruptIn)(req.inData).Unique)
  

•   if interreq != nil {
  

•       select {
  

•       case interreq.context.Interrupted <- true:
  

•       default:
  

•       }
  

•       ms.returnRequest(req)
  

•   } else {
  

•       log.Println("Requesting interrupt repeat")
  

•       time.Sleep(500 \* time.Millisecond)
  

•       req.status = Status(syscall.EAGAIN)
  

•       ms.write(req)
  

•       ms.returnRequest(req)
  

•   }
  

• } else {

•   if req.status.Ok() && req.handler.Func == nil {
  

•       log.Printf("Unimplemented opcode %v", operationName(req.inHeader.Opcode))
  

•       req.status = ENOSYS
  

•   }
  

• if req.status.Ok() {

•   req.handler.Func(ms, req)
  

• }

•   ms.pushInFlight(req)
  

• errNo := ms.write(req)
• if errNo != 0 {

•   log.Printf("writer: Write/Writev failed, err: %v. opcode: %v",
  

•       errNo, operationName(req.inHeader.Opcode))
  

•   if req.status.Ok() {
  

•       req.handler.Func(ms, req)
  

•   }
  

•   ms.popInFlight(req)
  

•   errNo := ms.write(req)
  

•   if errNo != 0 {
  

•       log.Printf("writer: Write/Writev failed, err: %v. opcode: %v",
  

•           errNo, operationName(req.inHeader.Opcode))
  

•   }
  

•   ms.returnRequest(req)
  

  }
• ms.returnRequest(req)
  }

func (ms *Server) allocOut(req *request, size uint32) []byte {
diff --git a/fuse/types.go b/fuse/types.go
index 7af20da..9f04435 100644
--- a/fuse/types.go
+++ b/fuse/types.go
@@ -48,5 +48,6 @@ type Owner raw.Owner
// Context contains assorted per-request data
type Context struct {
NodeId uint64

• Interrupted chan bool
  *raw.Context
  }

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/14
.

Han-Wen Nienhuys - [email protected] - http://www.xs4all.nl/~hanwen

from go-fuse.

Thank you for the prompt response.

The patch is just a proof of concept, we can change it any way you want.

Can you see if you put your inflight data protected by the same lock?

Sure

How about having a slice of outstanding requests, and in each request, annotate the request with the index in the slice. Then, for interrupt, do a linear search. This puts the cost of interrupt in the interrupt processing itself rather than in the normal path.

With this solution either:

• popInFlight just sets the entry in the slice to nil but pushInFlight does a linear scan of the slice looking for a nil spot to take over, or
• pushInFlight just appends to the array but popInFlight has to reslice the array when there is a group of nil entries at the end

Neither has zero impact on the common case. You will have to explain me your idea better :)

Unfortunately I'm leaving in about an hour and I won't be able to look into this for a couple of days, sorry. I wasn't expecting immediate feedback.

from go-fuse.

i think you can store a slice of free indexes. You pop from the free index slice when allocating, and push back when the request is released. See also

from go-fuse.

actually, IIRC, the request allocation already works like this, so you only have to add a slice of in-flight requests.

from go-fuse.

What is the status of this issue?

from go-fuse.

nothing has happened, IIRC.

from go-fuse.

If I understand correctly, this is fixed now. See #15 (comment) and fuse.Context now carries on cancellation channel (see e.g. c22e8d7).

from go-fuse.