From a7de8c87e88bafbd154a39f7d55a1d0158c32d6b Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 11:57:37 +1000 Subject: [PATCH 1/6] Made commonOp a bit less opaque. --- fuseops/common_op.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index 62c6fb7..b25a5c0 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -38,11 +38,14 @@ var fTraceByPID = flag.Bool( // A helper for embedding common behavior. type commonOp struct { - opType string - r bazilfuse.Request + opType string + bazilReq bazilfuse.Request + + // Dependencies. log func(int, string, ...interface{}) opsInFlight *sync.WaitGroup + // Context and tracing information. ctx context.Context report reqtrace.ReportFunc } @@ -145,15 +148,15 @@ func (o *commonOp) maybeTraceByPID( func (o *commonOp) init( ctx context.Context, opType reflect.Type, - r bazilfuse.Request, + bazilReq bazilfuse.Request, log func(int, string, ...interface{}), opsInFlight *sync.WaitGroup) { // Set up a context that reflects per-PID tracing if appropriate. - ctx = o.maybeTraceByPID(ctx, int(r.Hdr().Pid)) + ctx = o.maybeTraceByPID(ctx, int(bazilReq.Hdr().Pid)) // Initialize basic fields. o.opType = describeOpType(opType) - o.r = r + o.bazilReq = bazilReq o.log = log o.opsInFlight = opsInFlight @@ -162,7 +165,7 @@ func (o *commonOp) init( } func (o *commonOp) Header() OpHeader { - bh := o.r.Hdr() + bh := o.bazilReq.Hdr() return OpHeader{ Uid: bh.Uid, Gid: bh.Gid, @@ -190,19 +193,19 @@ func (o *commonOp) respondErr(err error) { o.opType, err) - o.r.RespondError(err) + o.bazilReq.RespondError(err) } // Respond with the supplied response struct, which must be accepted by a -// method called Respond on o.r. +// method called Respond on o.bazilReq. // -// Special case: nil means o.r.Respond accepts no parameters. +// Special case: nil means o.bazilReq.Respond accepts no parameters. func (o *commonOp) respond(resp interface{}) { // We were successful. o.report(nil) // Find the Respond method. - v := reflect.ValueOf(o.r) + v := reflect.ValueOf(o.bazilReq) respond := v.MethodByName("Respond") // Special case: handle successful ops with no response struct. From 66796316890c6e2cfcdda4261ec0c170f647213e Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 12:21:06 +1000 Subject: [PATCH 2/6] Refactored how op descriptions work. --- fuseops/common_op.go | 23 ++++++++++++++--------- fuseops/convert.go | 3 +-- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index b25a5c0..9ae47a1 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -38,7 +38,11 @@ var fTraceByPID = flag.Bool( // A helper for embedding common behavior. type commonOp struct { - opType string + // The op in which this struct is embedded, and a short description of it. + op Op + opDesc string + + // The underlying bazilfuse request for this op. bazilReq bazilfuse.Request // Dependencies. @@ -50,8 +54,8 @@ type commonOp struct { report reqtrace.ReportFunc } -func describeOpType(t reflect.Type) (desc string) { - name := t.String() +func describeOp(op Op) (desc string) { + name := reflect.TypeOf(op).String() // The usual case: a string that looks like "*fuseops.GetInodeAttributesOp". const prefix = "*fuseops." @@ -62,7 +66,7 @@ func describeOpType(t reflect.Type) (desc string) { } // Otherwise, it's not clear what to do. - desc = t.String() + desc = name return } @@ -147,7 +151,7 @@ func (o *commonOp) maybeTraceByPID( func (o *commonOp) init( ctx context.Context, - opType reflect.Type, + op Op, bazilReq bazilfuse.Request, log func(int, string, ...interface{}), opsInFlight *sync.WaitGroup) { @@ -155,13 +159,14 @@ func (o *commonOp) init( ctx = o.maybeTraceByPID(ctx, int(bazilReq.Hdr().Pid)) // Initialize basic fields. - o.opType = describeOpType(opType) + o.op = op + o.opDesc = describeOp(op) o.bazilReq = bazilReq o.log = log o.opsInFlight = opsInFlight // Set up a trace span for this op. - o.ctx, o.report = reqtrace.StartSpan(ctx, o.opType) + o.ctx, o.report = reqtrace.StartSpan(ctx, o.opDesc) } func (o *commonOp) Header() OpHeader { @@ -190,7 +195,7 @@ func (o *commonOp) respondErr(err error) { o.Logf( "-> (%s) error: %v", - o.opType, + o.opDesc, err) o.bazilReq.RespondError(err) @@ -210,7 +215,7 @@ func (o *commonOp) respond(resp interface{}) { // Special case: handle successful ops with no response struct. if resp == nil { - o.Logf("-> (%s) OK", o.opType) + o.Logf("-> (%s) OK", o.opDesc) respond.Call([]reflect.Value{}) return } diff --git a/fuseops/convert.go b/fuseops/convert.go index ef4a297..5a477f7 100644 --- a/fuseops/convert.go +++ b/fuseops/convert.go @@ -15,7 +15,6 @@ package fuseops import ( - "reflect" "sync" "time" @@ -216,7 +215,7 @@ func Convert( return } - co.init(parentCtx, reflect.TypeOf(o), r, logForOp, opsInFlight) + co.init(parentCtx, o, r, logForOp, opsInFlight) return } From 6868642c96ef2ed40af840144b9cc7f046e2a02f Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 12:21:22 +1000 Subject: [PATCH 3/6] Expanded description of PID spans. --- fuseops/common_op.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index 9ae47a1..faeddf4 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -140,7 +140,7 @@ func (o *commonOp) maybeTraceByPID( // Set up a new one and stick it in the map. var report reqtrace.ReportFunc - out, report = reqtrace.Trace(in, fmt.Sprintf("PID %v", pid)) + out, report = reqtrace.Trace(in, fmt.Sprintf("Requests from PID %v", pid)) gPIDMap[pid] = out // Ensure we close the trace and remove it from the map eventually. From d31b0cbd8148715ba509b325076725c2d868a1b9 Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 12:24:36 +1000 Subject: [PATCH 4/6] Refactored op descriptions again. --- fuseops/common_op.go | 44 +++++++++++++++++++++----------------------- fuseops/ops.go | 3 +++ 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index faeddf4..d4edbbf 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -39,8 +39,7 @@ var fTraceByPID = flag.Bool( // A helper for embedding common behavior. type commonOp struct { // The op in which this struct is embedded, and a short description of it. - op Op - opDesc string + op Op // The underlying bazilfuse request for this op. bazilReq bazilfuse.Request @@ -54,23 +53,6 @@ type commonOp struct { report reqtrace.ReportFunc } -func describeOp(op Op) (desc string) { - name := reflect.TypeOf(op).String() - - // The usual case: a string that looks like "*fuseops.GetInodeAttributesOp". - const prefix = "*fuseops." - const suffix = "Op" - if strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) { - desc = name[len(prefix) : len(name)-len(suffix)] - return - } - - // Otherwise, it's not clear what to do. - desc = name - - return -} - var gPIDMapMu sync.Mutex // A map from PID to a traced context for that PID. @@ -149,6 +131,23 @@ func (o *commonOp) maybeTraceByPID( return } +func (o *commonOp) ShortDesc() (desc string) { + name := reflect.TypeOf(o.op).String() + + // The usual case: a string that looks like "*fuseops.GetInodeAttributesOp". + const prefix = "*fuseops." + const suffix = "Op" + if strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) { + desc = name[len(prefix) : len(name)-len(suffix)] + return + } + + // Otherwise, it's not clear what to do. + desc = name + + return +} + func (o *commonOp) init( ctx context.Context, op Op, @@ -160,13 +159,12 @@ func (o *commonOp) init( // Initialize basic fields. o.op = op - o.opDesc = describeOp(op) o.bazilReq = bazilReq o.log = log o.opsInFlight = opsInFlight // Set up a trace span for this op. - o.ctx, o.report = reqtrace.StartSpan(ctx, o.opDesc) + o.ctx, o.report = reqtrace.StartSpan(ctx, o.ShortDesc()) } func (o *commonOp) Header() OpHeader { @@ -195,7 +193,7 @@ func (o *commonOp) respondErr(err error) { o.Logf( "-> (%s) error: %v", - o.opDesc, + o.ShortDesc(), err) o.bazilReq.RespondError(err) @@ -215,7 +213,7 @@ func (o *commonOp) respond(resp interface{}) { // Special case: handle successful ops with no response struct. if resp == nil { - o.Logf("-> (%s) OK", o.opDesc) + o.Logf("-> (%s) OK", o.ShortDesc()) respond.Call([]reflect.Value{}) return } diff --git a/fuseops/ops.go b/fuseops/ops.go index 8970798..976b530 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -29,6 +29,9 @@ import ( // to find particular concrete types, responding with fuse.ENOSYS if a type is // not supported. type Op interface { + // A short description of the op, to be used in logging. + ShortDesc() string + // Return the fields common to all operations. Header() OpHeader From 2dda1a5ea983a2f3f2b816cc6f348fcb94bcd95a Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 12:28:25 +1000 Subject: [PATCH 5/6] Include the object inode by default. --- fuseops/common_op.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index d4edbbf..cbb4206 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -132,18 +132,18 @@ func (o *commonOp) maybeTraceByPID( } func (o *commonOp) ShortDesc() (desc string) { - name := reflect.TypeOf(o.op).String() + opName := reflect.TypeOf(o.op).String() - // The usual case: a string that looks like "*fuseops.GetInodeAttributesOp". + // Attempt to better handle the usual case: a string that looks like + // "*fuseops.GetInodeAttributesOp". const prefix = "*fuseops." const suffix = "Op" - if strings.HasPrefix(name, prefix) && strings.HasSuffix(name, suffix) { - desc = name[len(prefix) : len(name)-len(suffix)] - return + if strings.HasPrefix(opName, prefix) && strings.HasSuffix(opName, suffix) { + opName = opName[len(prefix) : len(opName)-len(suffix)] } - // Otherwise, it's not clear what to do. - desc = name + // Include the inode number to which the op applies. + desc = fmt.Sprintf("%s(inode=%v)", opName, o.bazilReq.Hdr().Node) return } From 93de9409666a1bd9efd69df59e1cbb5ae68a107b Mon Sep 17 00:00:00 2001 From: Aaron Jacobs Date: Fri, 1 May 2015 12:38:03 +1000 Subject: [PATCH 6/6] Added some vanity op descriptions. --- fuseops/common_op.go | 6 +++--- fuseops/ops.go | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/fuseops/common_op.go b/fuseops/common_op.go index cbb4206..1a651d2 100644 --- a/fuseops/common_op.go +++ b/fuseops/common_op.go @@ -164,7 +164,7 @@ func (o *commonOp) init( o.opsInFlight = opsInFlight // Set up a trace span for this op. - o.ctx, o.report = reqtrace.StartSpan(ctx, o.ShortDesc()) + o.ctx, o.report = reqtrace.StartSpan(ctx, o.op.ShortDesc()) } func (o *commonOp) Header() OpHeader { @@ -193,7 +193,7 @@ func (o *commonOp) respondErr(err error) { o.Logf( "-> (%s) error: %v", - o.ShortDesc(), + o.op.ShortDesc(), err) o.bazilReq.RespondError(err) @@ -213,7 +213,7 @@ func (o *commonOp) respond(resp interface{}) { // Special case: handle successful ops with no response struct. if resp == nil { - o.Logf("-> (%s) OK", o.ShortDesc()) + o.Logf("-> (%s) OK", o.op.ShortDesc()) respond.Call([]reflect.Value{}) return } diff --git a/fuseops/ops.go b/fuseops/ops.go index 976b530..a683880 100644 --- a/fuseops/ops.go +++ b/fuseops/ops.go @@ -18,6 +18,7 @@ package fuseops import ( + "fmt" "os" "time" @@ -143,6 +144,11 @@ type LookUpInodeOp struct { Entry ChildInodeEntry } +func (o *LookUpInodeOp) ShortDesc() (desc string) { + desc = fmt.Sprintf("LookUpInode(parent=%v, name=%q)", o.Parent, o.Name) + return +} + func (o *LookUpInodeOp) Respond(err error) { defer o.commonOp.opsInFlight.Done() @@ -317,6 +323,11 @@ type MkDirOp struct { Entry ChildInodeEntry } +func (o *MkDirOp) ShortDesc() (desc string) { + desc = fmt.Sprintf("MkDir(parent=%v, name=%q)", o.Parent, o.Name) + return +} + func (o *MkDirOp) Respond(err error) { defer o.commonOp.opsInFlight.Done() @@ -374,6 +385,11 @@ type CreateFileOp struct { Handle HandleID } +func (o *CreateFileOp) ShortDesc() (desc string) { + desc = fmt.Sprintf("CreateFile(parent=%v, name=%q)", o.Parent, o.Name) + return +} + func (o *CreateFileOp) Respond(err error) { defer o.commonOp.opsInFlight.Done()