* [PATCH v2 0/4] afs: Miscellaneous fixes @ 2021-07-12 16:27 David Howells 2021-07-12 16:27 ` [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS David Howells ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: David Howells @ 2021-07-12 16:27 UTC (permalink / raw) To: linux-afs Cc: Steven Rostedt (VMware), Tom Rix, Abaci Robot, Andrew Morton, Marc Dionne, Alexey Dobriyan (SK hynix), Jiapeng Chong, dhowells, Marc Dionne, linux-fsdevel, linux-kernel Here are some fixes for AFS: (1) Fix a tracepoint that causes one of the tracing subsystem query files to crash if the module is loaded[1]. (2) Fix afs_writepages() to take account of whether the storage rpc actually succeeded when updating the cyclic writeback counter[2]. (3) Fix some error code propagation/handling[3]. (4) Fix place where afs_writepages() was setting writeback_index to a file position rather than a page index[4]. The patches can be found here: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes Changes ======= ver #2: - Fix an additional case of afs_writepages() setting writeback_index on error[4]. - Fix afs_writepages() setting writeback_index to a file pos[4]. David Link: https://lore.kernel.org/r/162430903582.2896199.6098150063997983353.stgit@warthog.procyon.org.uk/ [1] Link: https://lore.kernel.org/r/20210430155031.3287870-1-trix@redhat.com [2] Link: https://lore.kernel.org/r/1619691492-83866-1-git-send-email-jiapeng.chong@linux.alibaba.com [3] Link: https://lore.kernel.org/r/CAB9dFdvHsLsw7CMnB+4cgciWDSqVjuij4mH3TaXnHQB8sz5rHw@mail.gmail.com/ [4] --- David Howells (2): afs: Fix tracepoint string placement with built-in AFS afs: Fix setting of writeback_index Jiapeng Chong (1): afs: Remove redundant assignment to ret Tom Rix (1): afs: check function return fs/afs/dir.c | 10 ++++++---- fs/afs/write.c | 18 ++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS 2021-07-12 16:27 [PATCH v2 0/4] afs: Miscellaneous fixes David Howells @ 2021-07-12 16:27 ` David Howells 2021-07-12 19:27 ` Marc Dionne 2021-07-12 16:27 ` [PATCH v2 2/4] afs: check function return David Howells ` (2 subsequent siblings) 3 siblings, 1 reply; 8+ messages in thread From: David Howells @ 2021-07-12 16:27 UTC (permalink / raw) To: linux-afs Cc: Alexey Dobriyan (SK hynix), Steven Rostedt (VMware), Andrew Morton, dhowells, Marc Dionne, linux-fsdevel, linux-kernel To quote Alexey[1]: I was adding custom tracepoint to the kernel, grabbed full F34 kernel .config, disabled modules and booted whole shebang as VM kernel. Then did perf record -a -e ... It crashed: general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 RIP: 0010:t_show+0x22/0xd0 Then reproducer was narrowed to # cat /sys/kernel/tracing/printk_formats Original F34 kernel with modules didn't crash. So I started to disable options and after disabling AFS everything started working again. The root cause is that AFS was placing char arrays content into a section full of _pointers_ to strings with predictable consequences. Non canonical address 435f5346592e4243 is "CB.YFS_" which came from CM_NAME macro. Steps to reproduce: CONFIG_AFS=y CONFIG_TRACING=y # cat /sys/kernel/tracing/printk_formats Fix this by the following means: (1) Add enum->string translation tables in the event header with the AFS and YFS cache/callback manager operations listed by RPC operation ID. (2) Modify the afs_cb_call tracepoint to print the string from the translation table rather than using the string at the afs_call name pointer. (3) Switch translation table depending on the service we're being accessed as (AFS or YFS) in the tracepoint print clause. Will this cause problems to userspace utilities? Note that the symbolic representation of the YFS service ID isn't available to this header, so I've put it in as a number. I'm not sure if this is the best way to do this. (4) Remove the name wrangling (CM_NAME) macro and put the names directly into the afs_call_type structs in cmservice.c. Fixes: 8e8d7f13b6d5a9 ("afs: Add some tracepoints") Reported-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> cc: Andrew Morton <akpm@linux-foundation.org> cc: linux-afs@lists.infradead.org Link: https://lore.kernel.org/r/YLAXfvZ+rObEOdc%2F@localhost.localdomain/ [1] Link: https://lore.kernel.org/r/643721.1623754699@warthog.procyon.org.uk/ Link: https://lore.kernel.org/r/162430903582.2896199.6098150063997983353.stgit@warthog.procyon.org.uk/ # v1 Link: https://lore.kernel.org/r/162609463957.3133237.15916579353149746363.stgit@warthog.procyon.org.uk/ # v1 (repost) --- fs/afs/cmservice.c | 25 +++++----------- include/trace/events/afs.h | 67 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c index d3c6bb22c5f4..a3f5de28be79 100644 --- a/fs/afs/cmservice.c +++ b/fs/afs/cmservice.c @@ -29,16 +29,11 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *); static int afs_deliver_yfs_cb_callback(struct afs_call *); -#define CM_NAME(name) \ - char afs_SRXCB##name##_name[] __tracepoint_string = \ - "CB." #name - /* * CB.CallBack operation type */ -static CM_NAME(CallBack); static const struct afs_call_type afs_SRXCBCallBack = { - .name = afs_SRXCBCallBack_name, + .name = "CB.CallBack", .deliver = afs_deliver_cb_callback, .destructor = afs_cm_destructor, .work = SRXAFSCB_CallBack, @@ -47,9 +42,8 @@ static const struct afs_call_type afs_SRXCBCallBack = { /* * CB.InitCallBackState operation type */ -static CM_NAME(InitCallBackState); static const struct afs_call_type afs_SRXCBInitCallBackState = { - .name = afs_SRXCBInitCallBackState_name, + .name = "CB.InitCallBackState", .deliver = afs_deliver_cb_init_call_back_state, .destructor = afs_cm_destructor, .work = SRXAFSCB_InitCallBackState, @@ -58,9 +52,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState = { /* * CB.InitCallBackState3 operation type */ -static CM_NAME(InitCallBackState3); static const struct afs_call_type afs_SRXCBInitCallBackState3 = { - .name = afs_SRXCBInitCallBackState3_name, + .name = "CB.InitCallBackState3", .deliver = afs_deliver_cb_init_call_back_state3, .destructor = afs_cm_destructor, .work = SRXAFSCB_InitCallBackState, @@ -69,9 +62,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState3 = { /* * CB.Probe operation type */ -static CM_NAME(Probe); static const struct afs_call_type afs_SRXCBProbe = { - .name = afs_SRXCBProbe_name, + .name = "CB.Probe", .deliver = afs_deliver_cb_probe, .destructor = afs_cm_destructor, .work = SRXAFSCB_Probe, @@ -80,9 +72,8 @@ static const struct afs_call_type afs_SRXCBProbe = { /* * CB.ProbeUuid operation type */ -static CM_NAME(ProbeUuid); static const struct afs_call_type afs_SRXCBProbeUuid = { - .name = afs_SRXCBProbeUuid_name, + .name = "CB.ProbeUuid", .deliver = afs_deliver_cb_probe_uuid, .destructor = afs_cm_destructor, .work = SRXAFSCB_ProbeUuid, @@ -91,9 +82,8 @@ static const struct afs_call_type afs_SRXCBProbeUuid = { /* * CB.TellMeAboutYourself operation type */ -static CM_NAME(TellMeAboutYourself); static const struct afs_call_type afs_SRXCBTellMeAboutYourself = { - .name = afs_SRXCBTellMeAboutYourself_name, + .name = "CB.TellMeAboutYourself", .deliver = afs_deliver_cb_tell_me_about_yourself, .destructor = afs_cm_destructor, .work = SRXAFSCB_TellMeAboutYourself, @@ -102,9 +92,8 @@ static const struct afs_call_type afs_SRXCBTellMeAboutYourself = { /* * YFS CB.CallBack operation type */ -static CM_NAME(YFS_CallBack); static const struct afs_call_type afs_SRXYFSCB_CallBack = { - .name = afs_SRXCBYFS_CallBack_name, + .name = "YFSCB.CallBack", .deliver = afs_deliver_yfs_cb_callback, .destructor = afs_cm_destructor, .work = SRXAFSCB_CallBack, diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h index 3ccf591b2374..9f73ed2cf061 100644 --- a/include/trace/events/afs.h +++ b/include/trace/events/afs.h @@ -174,6 +174,34 @@ enum afs_vl_operation { afs_VL_GetCapabilities = 65537, /* AFS Get VL server capabilities */ }; +enum afs_cm_operation { + afs_CB_CallBack = 204, /* AFS break callback promises */ + afs_CB_InitCallBackState = 205, /* AFS initialise callback state */ + afs_CB_Probe = 206, /* AFS probe client */ + afs_CB_GetLock = 207, /* AFS get contents of CM lock table */ + afs_CB_GetCE = 208, /* AFS get cache file description */ + afs_CB_GetXStatsVersion = 209, /* AFS get version of extended statistics */ + afs_CB_GetXStats = 210, /* AFS get contents of extended statistics data */ + afs_CB_InitCallBackState3 = 213, /* AFS initialise callback state, version 3 */ + afs_CB_ProbeUuid = 214, /* AFS check the client hasn't rebooted */ +}; + +enum yfs_cm_operation { + yfs_CB_Probe = 206, /* YFS probe client */ + yfs_CB_GetLock = 207, /* YFS get contents of CM lock table */ + yfs_CB_XStatsVersion = 209, /* YFS get version of extended statistics */ + yfs_CB_GetXStats = 210, /* YFS get contents of extended statistics data */ + yfs_CB_InitCallBackState3 = 213, /* YFS initialise callback state, version 3 */ + yfs_CB_ProbeUuid = 214, /* YFS check the client hasn't rebooted */ + yfs_CB_GetServerPrefs = 215, + yfs_CB_GetCellServDV = 216, + yfs_CB_GetLocalCell = 217, + yfs_CB_GetCacheConfig = 218, + yfs_CB_GetCellByNum = 65537, + yfs_CB_TellMeAboutYourself = 65538, /* get client capabilities */ + yfs_CB_CallBack = 64204, +}; + enum afs_edit_dir_op { afs_edit_dir_create, afs_edit_dir_create_error, @@ -436,6 +464,32 @@ enum afs_cb_break_reason { EM(afs_YFSVL_GetCellName, "YFSVL.GetCellName") \ E_(afs_VL_GetCapabilities, "VL.GetCapabilities") +#define afs_cm_operations \ + EM(afs_CB_CallBack, "CB.CallBack") \ + EM(afs_CB_InitCallBackState, "CB.InitCallBackState") \ + EM(afs_CB_Probe, "CB.Probe") \ + EM(afs_CB_GetLock, "CB.GetLock") \ + EM(afs_CB_GetCE, "CB.GetCE") \ + EM(afs_CB_GetXStatsVersion, "CB.GetXStatsVersion") \ + EM(afs_CB_GetXStats, "CB.GetXStats") \ + EM(afs_CB_InitCallBackState3, "CB.InitCallBackState3") \ + E_(afs_CB_ProbeUuid, "CB.ProbeUuid") + +#define yfs_cm_operations \ + EM(yfs_CB_Probe, "YFSCB.Probe") \ + EM(yfs_CB_GetLock, "YFSCB.GetLock") \ + EM(yfs_CB_XStatsVersion, "YFSCB.XStatsVersion") \ + EM(yfs_CB_GetXStats, "YFSCB.GetXStats") \ + EM(yfs_CB_InitCallBackState3, "YFSCB.InitCallBackState3") \ + EM(yfs_CB_ProbeUuid, "YFSCB.ProbeUuid") \ + EM(yfs_CB_GetServerPrefs, "YFSCB.GetServerPrefs") \ + EM(yfs_CB_GetCellServDV, "YFSCB.GetCellServDV") \ + EM(yfs_CB_GetLocalCell, "YFSCB.GetLocalCell") \ + EM(yfs_CB_GetCacheConfig, "YFSCB.GetCacheConfig") \ + EM(yfs_CB_GetCellByNum, "YFSCB.GetCellByNum") \ + EM(yfs_CB_TellMeAboutYourself, "YFSCB.TellMeAboutYourself") \ + E_(yfs_CB_CallBack, "YFSCB.CallBack") + #define afs_edit_dir_ops \ EM(afs_edit_dir_create, "create") \ EM(afs_edit_dir_create_error, "c_fail") \ @@ -569,6 +623,8 @@ afs_server_traces; afs_cell_traces; afs_fs_operations; afs_vl_operations; +afs_cm_operations; +yfs_cm_operations; afs_edit_dir_ops; afs_edit_dir_reasons; afs_eproto_causes; @@ -649,20 +705,21 @@ TRACE_EVENT(afs_cb_call, TP_STRUCT__entry( __field(unsigned int, call ) - __field(const char *, name ) __field(u32, op ) + __field(u16, service_id ) ), TP_fast_assign( __entry->call = call->debug_id; - __entry->name = call->type->name; __entry->op = call->operation_ID; + __entry->service_id = call->service_id; ), - TP_printk("c=%08x %s o=%u", + TP_printk("c=%08x %s", __entry->call, - __entry->name, - __entry->op) + __entry->service_id == 2501 ? + __print_symbolic(__entry->op, yfs_cm_operations) : + __print_symbolic(__entry->op, afs_cm_operations)) ); TRACE_EVENT(afs_call, ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS 2021-07-12 16:27 ` [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS David Howells @ 2021-07-12 19:27 ` Marc Dionne 0 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2021-07-12 19:27 UTC (permalink / raw) To: David Howells Cc: linux-afs, Alexey Dobriyan (SK hynix), Steven Rostedt (VMware), Andrew Morton, linux-fsdevel, Linux Kernel Mailing List On Mon, Jul 12, 2021 at 1:28 PM David Howells <dhowells@redhat.com> wrote: > > To quote Alexey[1]: > > I was adding custom tracepoint to the kernel, grabbed full F34 kernel > .config, disabled modules and booted whole shebang as VM kernel. > > Then did > > perf record -a -e ... > > It crashed: > > general protection fault, probably for non-canonical address 0x435f5346592e4243: 0000 [#1] SMP PTI > CPU: 1 PID: 842 Comm: cat Not tainted 5.12.6+ #26 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-1.fc33 04/01/2014 > RIP: 0010:t_show+0x22/0xd0 > > Then reproducer was narrowed to > > # cat /sys/kernel/tracing/printk_formats > > Original F34 kernel with modules didn't crash. > > So I started to disable options and after disabling AFS everything > started working again. > > The root cause is that AFS was placing char arrays content into a > section full of _pointers_ to strings with predictable consequences. > > Non canonical address 435f5346592e4243 is "CB.YFS_" which came from > CM_NAME macro. > > Steps to reproduce: > > CONFIG_AFS=y > CONFIG_TRACING=y > > # cat /sys/kernel/tracing/printk_formats > > Fix this by the following means: > > (1) Add enum->string translation tables in the event header with the AFS > and YFS cache/callback manager operations listed by RPC operation ID. > > (2) Modify the afs_cb_call tracepoint to print the string from the > translation table rather than using the string at the afs_call name > pointer. > > (3) Switch translation table depending on the service we're being accessed > as (AFS or YFS) in the tracepoint print clause. Will this cause > problems to userspace utilities? > > Note that the symbolic representation of the YFS service ID isn't > available to this header, so I've put it in as a number. I'm not sure > if this is the best way to do this. > > (4) Remove the name wrangling (CM_NAME) macro and put the names directly > into the afs_call_type structs in cmservice.c. > > Fixes: 8e8d7f13b6d5a9 ("afs: Add some tracepoints") > Reported-by: Alexey Dobriyan (SK hynix) <adobriyan@gmail.com> > Signed-off-by: David Howells <dhowells@redhat.com> > Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > cc: Andrew Morton <akpm@linux-foundation.org> > cc: linux-afs@lists.infradead.org > Link: https://lore.kernel.org/r/YLAXfvZ+rObEOdc%2F@localhost.localdomain/ [1] > Link: https://lore.kernel.org/r/643721.1623754699@warthog.procyon.org.uk/ > Link: https://lore.kernel.org/r/162430903582.2896199.6098150063997983353.stgit@warthog.procyon.org.uk/ # v1 > Link: https://lore.kernel.org/r/162609463957.3133237.15916579353149746363.stgit@warthog.procyon.org.uk/ # v1 (repost) > --- > > fs/afs/cmservice.c | 25 +++++----------- > include/trace/events/afs.h | 67 +++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 69 insertions(+), 23 deletions(-) > > diff --git a/fs/afs/cmservice.c b/fs/afs/cmservice.c > index d3c6bb22c5f4..a3f5de28be79 100644 > --- a/fs/afs/cmservice.c > +++ b/fs/afs/cmservice.c > @@ -29,16 +29,11 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *); > > static int afs_deliver_yfs_cb_callback(struct afs_call *); > > -#define CM_NAME(name) \ > - char afs_SRXCB##name##_name[] __tracepoint_string = \ > - "CB." #name > - > /* > * CB.CallBack operation type > */ > -static CM_NAME(CallBack); > static const struct afs_call_type afs_SRXCBCallBack = { > - .name = afs_SRXCBCallBack_name, > + .name = "CB.CallBack", > .deliver = afs_deliver_cb_callback, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_CallBack, > @@ -47,9 +42,8 @@ static const struct afs_call_type afs_SRXCBCallBack = { > /* > * CB.InitCallBackState operation type > */ > -static CM_NAME(InitCallBackState); > static const struct afs_call_type afs_SRXCBInitCallBackState = { > - .name = afs_SRXCBInitCallBackState_name, > + .name = "CB.InitCallBackState", > .deliver = afs_deliver_cb_init_call_back_state, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_InitCallBackState, > @@ -58,9 +52,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState = { > /* > * CB.InitCallBackState3 operation type > */ > -static CM_NAME(InitCallBackState3); > static const struct afs_call_type afs_SRXCBInitCallBackState3 = { > - .name = afs_SRXCBInitCallBackState3_name, > + .name = "CB.InitCallBackState3", > .deliver = afs_deliver_cb_init_call_back_state3, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_InitCallBackState, > @@ -69,9 +62,8 @@ static const struct afs_call_type afs_SRXCBInitCallBackState3 = { > /* > * CB.Probe operation type > */ > -static CM_NAME(Probe); > static const struct afs_call_type afs_SRXCBProbe = { > - .name = afs_SRXCBProbe_name, > + .name = "CB.Probe", > .deliver = afs_deliver_cb_probe, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_Probe, > @@ -80,9 +72,8 @@ static const struct afs_call_type afs_SRXCBProbe = { > /* > * CB.ProbeUuid operation type > */ > -static CM_NAME(ProbeUuid); > static const struct afs_call_type afs_SRXCBProbeUuid = { > - .name = afs_SRXCBProbeUuid_name, > + .name = "CB.ProbeUuid", > .deliver = afs_deliver_cb_probe_uuid, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_ProbeUuid, > @@ -91,9 +82,8 @@ static const struct afs_call_type afs_SRXCBProbeUuid = { > /* > * CB.TellMeAboutYourself operation type > */ > -static CM_NAME(TellMeAboutYourself); > static const struct afs_call_type afs_SRXCBTellMeAboutYourself = { > - .name = afs_SRXCBTellMeAboutYourself_name, > + .name = "CB.TellMeAboutYourself", > .deliver = afs_deliver_cb_tell_me_about_yourself, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_TellMeAboutYourself, > @@ -102,9 +92,8 @@ static const struct afs_call_type afs_SRXCBTellMeAboutYourself = { > /* > * YFS CB.CallBack operation type > */ > -static CM_NAME(YFS_CallBack); > static const struct afs_call_type afs_SRXYFSCB_CallBack = { > - .name = afs_SRXCBYFS_CallBack_name, > + .name = "YFSCB.CallBack", > .deliver = afs_deliver_yfs_cb_callback, > .destructor = afs_cm_destructor, > .work = SRXAFSCB_CallBack, > diff --git a/include/trace/events/afs.h b/include/trace/events/afs.h > index 3ccf591b2374..9f73ed2cf061 100644 > --- a/include/trace/events/afs.h > +++ b/include/trace/events/afs.h > @@ -174,6 +174,34 @@ enum afs_vl_operation { > afs_VL_GetCapabilities = 65537, /* AFS Get VL server capabilities */ > }; > > +enum afs_cm_operation { > + afs_CB_CallBack = 204, /* AFS break callback promises */ > + afs_CB_InitCallBackState = 205, /* AFS initialise callback state */ > + afs_CB_Probe = 206, /* AFS probe client */ > + afs_CB_GetLock = 207, /* AFS get contents of CM lock table */ > + afs_CB_GetCE = 208, /* AFS get cache file description */ > + afs_CB_GetXStatsVersion = 209, /* AFS get version of extended statistics */ > + afs_CB_GetXStats = 210, /* AFS get contents of extended statistics data */ > + afs_CB_InitCallBackState3 = 213, /* AFS initialise callback state, version 3 */ > + afs_CB_ProbeUuid = 214, /* AFS check the client hasn't rebooted */ > +}; > + > +enum yfs_cm_operation { > + yfs_CB_Probe = 206, /* YFS probe client */ > + yfs_CB_GetLock = 207, /* YFS get contents of CM lock table */ > + yfs_CB_XStatsVersion = 209, /* YFS get version of extended statistics */ > + yfs_CB_GetXStats = 210, /* YFS get contents of extended statistics data */ > + yfs_CB_InitCallBackState3 = 213, /* YFS initialise callback state, version 3 */ > + yfs_CB_ProbeUuid = 214, /* YFS check the client hasn't rebooted */ > + yfs_CB_GetServerPrefs = 215, > + yfs_CB_GetCellServDV = 216, > + yfs_CB_GetLocalCell = 217, > + yfs_CB_GetCacheConfig = 218, > + yfs_CB_GetCellByNum = 65537, > + yfs_CB_TellMeAboutYourself = 65538, /* get client capabilities */ > + yfs_CB_CallBack = 64204, > +}; > + > enum afs_edit_dir_op { > afs_edit_dir_create, > afs_edit_dir_create_error, > @@ -436,6 +464,32 @@ enum afs_cb_break_reason { > EM(afs_YFSVL_GetCellName, "YFSVL.GetCellName") \ > E_(afs_VL_GetCapabilities, "VL.GetCapabilities") > > +#define afs_cm_operations \ > + EM(afs_CB_CallBack, "CB.CallBack") \ > + EM(afs_CB_InitCallBackState, "CB.InitCallBackState") \ > + EM(afs_CB_Probe, "CB.Probe") \ > + EM(afs_CB_GetLock, "CB.GetLock") \ > + EM(afs_CB_GetCE, "CB.GetCE") \ > + EM(afs_CB_GetXStatsVersion, "CB.GetXStatsVersion") \ > + EM(afs_CB_GetXStats, "CB.GetXStats") \ > + EM(afs_CB_InitCallBackState3, "CB.InitCallBackState3") \ > + E_(afs_CB_ProbeUuid, "CB.ProbeUuid") > + > +#define yfs_cm_operations \ > + EM(yfs_CB_Probe, "YFSCB.Probe") \ > + EM(yfs_CB_GetLock, "YFSCB.GetLock") \ > + EM(yfs_CB_XStatsVersion, "YFSCB.XStatsVersion") \ > + EM(yfs_CB_GetXStats, "YFSCB.GetXStats") \ > + EM(yfs_CB_InitCallBackState3, "YFSCB.InitCallBackState3") \ > + EM(yfs_CB_ProbeUuid, "YFSCB.ProbeUuid") \ > + EM(yfs_CB_GetServerPrefs, "YFSCB.GetServerPrefs") \ > + EM(yfs_CB_GetCellServDV, "YFSCB.GetCellServDV") \ > + EM(yfs_CB_GetLocalCell, "YFSCB.GetLocalCell") \ > + EM(yfs_CB_GetCacheConfig, "YFSCB.GetCacheConfig") \ > + EM(yfs_CB_GetCellByNum, "YFSCB.GetCellByNum") \ > + EM(yfs_CB_TellMeAboutYourself, "YFSCB.TellMeAboutYourself") \ > + E_(yfs_CB_CallBack, "YFSCB.CallBack") > + > #define afs_edit_dir_ops \ > EM(afs_edit_dir_create, "create") \ > EM(afs_edit_dir_create_error, "c_fail") \ > @@ -569,6 +623,8 @@ afs_server_traces; > afs_cell_traces; > afs_fs_operations; > afs_vl_operations; > +afs_cm_operations; > +yfs_cm_operations; > afs_edit_dir_ops; > afs_edit_dir_reasons; > afs_eproto_causes; > @@ -649,20 +705,21 @@ TRACE_EVENT(afs_cb_call, > > TP_STRUCT__entry( > __field(unsigned int, call ) > - __field(const char *, name ) > __field(u32, op ) > + __field(u16, service_id ) > ), > > TP_fast_assign( > __entry->call = call->debug_id; > - __entry->name = call->type->name; > __entry->op = call->operation_ID; > + __entry->service_id = call->service_id; > ), > > - TP_printk("c=%08x %s o=%u", > + TP_printk("c=%08x %s", > __entry->call, > - __entry->name, > - __entry->op) > + __entry->service_id == 2501 ? > + __print_symbolic(__entry->op, yfs_cm_operations) : > + __print_symbolic(__entry->op, afs_cm_operations)) > ); > > TRACE_EVENT(afs_call, > > Reviewed-by: Marc Dionne <marc.dionne@auristor.com> If you wanted to avoid having the hard coded yfs service ID there, you could pass in a boolean "is yfs" value and let the caller set it based on the service ID, or even have two separate trace events. But seems fine as is. Marc ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] afs: check function return 2021-07-12 16:27 [PATCH v2 0/4] afs: Miscellaneous fixes David Howells 2021-07-12 16:27 ` [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS David Howells @ 2021-07-12 16:27 ` David Howells 2021-07-21 12:59 ` Marc Dionne 2021-07-12 16:28 ` [PATCH v2 3/4] afs: Fix setting of writeback_index David Howells 2021-07-12 16:28 ` [PATCH v2 4/4] afs: Remove redundant assignment to ret David Howells 3 siblings, 1 reply; 8+ messages in thread From: David Howells @ 2021-07-12 16:27 UTC (permalink / raw) To: linux-afs; +Cc: Tom Rix, dhowells, Marc Dionne, linux-fsdevel, linux-kernel From: Tom Rix <trix@redhat.com> Static analysis reports this problem write.c:773:29: warning: Assigned value is garbage or undefined mapping->writeback_index = next; ^ ~~~~ The call to afs_writepages_region() can return without setting next. So check the function return before using next. Changes: ver #2: - Need to fix the range_cyclic case also[1]. Fixes: e87b03f5830e ("afs: Prepare for use of THPs") Signed-off-by: Tom Rix <trix@redhat.com> Signed-off-by: David Howells <dhowells@redhat.com> Link: https://lore.kernel.org/r/20210430155031.3287870-1-trix@redhat.com Link: https://lore.kernel.org/r/162609464716.3133237.10354897554363093252.stgit@warthog.procyon.org.uk/ # v1 Link: https://lore.kernel.org/r/CAB9dFdvHsLsw7CMnB+4cgciWDSqVjuij4mH3TaXnHQB8sz5rHw@mail.gmail.com/ [1] --- fs/afs/write.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/fs/afs/write.c b/fs/afs/write.c index 3104b62c2082..1ed62e0ccfe5 100644 --- a/fs/afs/write.c +++ b/fs/afs/write.c @@ -771,13 +771,19 @@ int afs_writepages(struct address_space *mapping, if (wbc->range_cyclic) { start = mapping->writeback_index * PAGE_SIZE; ret = afs_writepages_region(mapping, wbc, start, LLONG_MAX, &next); - if (start > 0 && wbc->nr_to_write > 0 && ret == 0) - ret = afs_writepages_region(mapping, wbc, 0, start, - &next); - mapping->writeback_index = next / PAGE_SIZE; + if (ret == 0) { + mapping->writeback_index = next / PAGE_SIZE; + if (start > 0 && wbc->nr_to_write > 0) { + ret = afs_writepages_region(mapping, wbc, 0, + start, &next); + if (ret == 0) + mapping->writeback_index = + next / PAGE_SIZE; + } + } } else if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) { ret = afs_writepages_region(mapping, wbc, 0, LLONG_MAX, &next); - if (wbc->nr_to_write > 0) + if (wbc->nr_to_write > 0 && ret == 0) mapping->writeback_index = next; } else { ret = afs_writepages_region(mapping, wbc, ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] afs: check function return 2021-07-12 16:27 ` [PATCH v2 2/4] afs: check function return David Howells @ 2021-07-21 12:59 ` Marc Dionne 0 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2021-07-21 12:59 UTC (permalink / raw) To: David Howells Cc: linux-afs, Tom Rix, linux-fsdevel, Linux Kernel Mailing List On Mon, Jul 12, 2021 at 1:28 PM David Howells <dhowells@redhat.com> wrote: > > From: Tom Rix <trix@redhat.com> > > Static analysis reports this problem > > write.c:773:29: warning: Assigned value is garbage or undefined > mapping->writeback_index = next; > ^ ~~~~ > The call to afs_writepages_region() can return without setting > next. So check the function return before using next. > > Changes: > ver #2: > - Need to fix the range_cyclic case also[1]. > > Fixes: e87b03f5830e ("afs: Prepare for use of THPs") > Signed-off-by: Tom Rix <trix@redhat.com> > Signed-off-by: David Howells <dhowells@redhat.com> > Link: https://lore.kernel.org/r/20210430155031.3287870-1-trix@redhat.com > Link: https://lore.kernel.org/r/162609464716.3133237.10354897554363093252.stgit@warthog.procyon.org.uk/ # v1 > Link: https://lore.kernel.org/r/CAB9dFdvHsLsw7CMnB+4cgciWDSqVjuij4mH3TaXnHQB8sz5rHw@mail.gmail.com/ [1] > --- > > fs/afs/write.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/afs/write.c b/fs/afs/write.c > index 3104b62c2082..1ed62e0ccfe5 100644 > --- a/fs/afs/write.c > +++ b/fs/afs/write.c > @@ -771,13 +771,19 @@ int afs_writepages(struct address_space *mapping, > if (wbc->range_cyclic) { > start = mapping->writeback_index * PAGE_SIZE; > ret = afs_writepages_region(mapping, wbc, start, LLONG_MAX, &next); > - if (start > 0 && wbc->nr_to_write > 0 && ret == 0) > - ret = afs_writepages_region(mapping, wbc, 0, start, > - &next); > - mapping->writeback_index = next / PAGE_SIZE; > + if (ret == 0) { > + mapping->writeback_index = next / PAGE_SIZE; > + if (start > 0 && wbc->nr_to_write > 0) { > + ret = afs_writepages_region(mapping, wbc, 0, > + start, &next); > + if (ret == 0) > + mapping->writeback_index = > + next / PAGE_SIZE; > + } > + } > } else if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) { > ret = afs_writepages_region(mapping, wbc, 0, LLONG_MAX, &next); > - if (wbc->nr_to_write > 0) > + if (wbc->nr_to_write > 0 && ret == 0) > mapping->writeback_index = next; > } else { > ret = afs_writepages_region(mapping, wbc, > > Reviewed-by: Marc Dionne <marc.dionne@auristor.com> Marc ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] afs: Fix setting of writeback_index 2021-07-12 16:27 [PATCH v2 0/4] afs: Miscellaneous fixes David Howells 2021-07-12 16:27 ` [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS David Howells 2021-07-12 16:27 ` [PATCH v2 2/4] afs: check function return David Howells @ 2021-07-12 16:28 ` David Howells 2021-07-21 12:52 ` Marc Dionne 2021-07-12 16:28 ` [PATCH v2 4/4] afs: Remove redundant assignment to ret David Howells 3 siblings, 1 reply; 8+ messages in thread From: David Howells @ 2021-07-12 16:28 UTC (permalink / raw) To: linux-afs; +Cc: Marc Dionne, dhowells, Marc Dionne, linux-fsdevel, linux-kernel Fix afs_writepages() to always set mapping->writeback_index to a page index and not a byte position[1]. Fixes: 31143d5d515e ("AFS: implement basic file write support") Reported-by: Marc Dionne <marc.dionne@auristor.com> Signed-off-by: David Howells <dhowells@redhat.com> Link: https://lore.kernel.org/r/CAB9dFdvHsLsw7CMnB+4cgciWDSqVjuij4mH3TaXnHQB8sz5rHw@mail.gmail.com/ [1] cc: linux-afs@lists.infradead.org --- fs/afs/write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/afs/write.c b/fs/afs/write.c index 1ed62e0ccfe5..c0534697268e 100644 --- a/fs/afs/write.c +++ b/fs/afs/write.c @@ -784,7 +784,7 @@ int afs_writepages(struct address_space *mapping, } else if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) { ret = afs_writepages_region(mapping, wbc, 0, LLONG_MAX, &next); if (wbc->nr_to_write > 0 && ret == 0) - mapping->writeback_index = next; + mapping->writeback_index = next / PAGE_SIZE; } else { ret = afs_writepages_region(mapping, wbc, wbc->range_start, wbc->range_end, &next); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/4] afs: Fix setting of writeback_index 2021-07-12 16:28 ` [PATCH v2 3/4] afs: Fix setting of writeback_index David Howells @ 2021-07-21 12:52 ` Marc Dionne 0 siblings, 0 replies; 8+ messages in thread From: Marc Dionne @ 2021-07-21 12:52 UTC (permalink / raw) To: David Howells; +Cc: linux-afs, linux-fsdevel, Linux Kernel Mailing List On Mon, Jul 12, 2021 at 1:28 PM David Howells <dhowells@redhat.com> wrote: > > Fix afs_writepages() to always set mapping->writeback_index to a page index > and not a byte position[1]. > > Fixes: 31143d5d515e ("AFS: implement basic file write support") > Reported-by: Marc Dionne <marc.dionne@auristor.com> > Signed-off-by: David Howells <dhowells@redhat.com> > Link: https://lore.kernel.org/r/CAB9dFdvHsLsw7CMnB+4cgciWDSqVjuij4mH3TaXnHQB8sz5rHw@mail.gmail.com/ [1] > cc: linux-afs@lists.infradead.org > --- > > fs/afs/write.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/afs/write.c b/fs/afs/write.c > index 1ed62e0ccfe5..c0534697268e 100644 > --- a/fs/afs/write.c > +++ b/fs/afs/write.c > @@ -784,7 +784,7 @@ int afs_writepages(struct address_space *mapping, > } else if (wbc->range_start == 0 && wbc->range_end == LLONG_MAX) { > ret = afs_writepages_region(mapping, wbc, 0, LLONG_MAX, &next); > if (wbc->nr_to_write > 0 && ret == 0) > - mapping->writeback_index = next; > + mapping->writeback_index = next / PAGE_SIZE; > } else { > ret = afs_writepages_region(mapping, wbc, > wbc->range_start, wbc->range_end, &next); > > > Reviewed-by: Marc Dionne <marc.dionne@auristor.com> Marc ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] afs: Remove redundant assignment to ret 2021-07-12 16:27 [PATCH v2 0/4] afs: Miscellaneous fixes David Howells ` (2 preceding siblings ...) 2021-07-12 16:28 ` [PATCH v2 3/4] afs: Fix setting of writeback_index David Howells @ 2021-07-12 16:28 ` David Howells 3 siblings, 0 replies; 8+ messages in thread From: David Howells @ 2021-07-12 16:28 UTC (permalink / raw) To: linux-afs Cc: Abaci Robot, Jiapeng Chong, Marc Dionne, dhowells, Marc Dionne, linux-fsdevel, linux-kernel From: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Variable ret is set to -ENOENT and -ENOMEM but this value is never read as it is overwritten or not used later on, hence it is a redundant assignment and can be removed. Cleans up the following clang-analyzer warning: fs/afs/dir.c:2014:4: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]. fs/afs/dir.c:659:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]. [DH made the following modifications: - In afs_rename(), -ENOMEM should be placed in op->error instead of ret, rather than the assignment being removed entirely. afs_put_operation() will pick it up from there and return it. - If afs_sillyrename() fails, its error code should be placed in op->error rather than in ret also. ] Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept") Reported-by: Abaci Robot <abaci@linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Marc Dionne <marc.dionne@auristor.com> cc: linux-afs@lists.infradead.org Link: https://lore.kernel.org/r/1619691492-83866-1-git-send-email-jiapeng.chong@linux.alibaba.com Link: https://lore.kernel.org/r/162609465444.3133237.7562832521724298900.stgit@warthog.procyon.org.uk/ # v1 --- fs/afs/dir.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/afs/dir.c b/fs/afs/dir.c index 78719f2f567e..ac829e63c570 100644 --- a/fs/afs/dir.c +++ b/fs/afs/dir.c @@ -656,7 +656,6 @@ static int afs_do_lookup_one(struct inode *dir, struct dentry *dentry, return ret; } - ret = -ENOENT; if (!cookie.found) { _leave(" = -ENOENT [not found]"); return -ENOENT; @@ -2020,17 +2019,20 @@ static int afs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, if (d_count(new_dentry) > 2) { /* copy the target dentry's name */ - ret = -ENOMEM; op->rename.tmp = d_alloc(new_dentry->d_parent, &new_dentry->d_name); - if (!op->rename.tmp) + if (!op->rename.tmp) { + op->error = -ENOMEM; goto error; + } ret = afs_sillyrename(new_dvnode, AFS_FS_I(d_inode(new_dentry)), new_dentry, op->key); - if (ret) + if (ret) { + op->error = ret; goto error; + } op->dentry_2 = op->rename.tmp; op->rename.rehash = NULL; ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-21 12:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-12 16:27 [PATCH v2 0/4] afs: Miscellaneous fixes David Howells 2021-07-12 16:27 ` [PATCH v2 1/4] afs: Fix tracepoint string placement with built-in AFS David Howells 2021-07-12 19:27 ` Marc Dionne 2021-07-12 16:27 ` [PATCH v2 2/4] afs: check function return David Howells 2021-07-21 12:59 ` Marc Dionne 2021-07-12 16:28 ` [PATCH v2 3/4] afs: Fix setting of writeback_index David Howells 2021-07-21 12:52 ` Marc Dionne 2021-07-12 16:28 ` [PATCH v2 4/4] afs: Remove redundant assignment to ret David Howells
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.