[PATCH] fs: export emergency_sync

[PATCH] fs: export emergency_sync

[Rob Barnes]

emergency_sync forces a filesystem sync in emergency situations.
Export this function so it can be used by modules.

Signed-off-by: Rob Barnes <robbarnes@google.com>
---

 fs/sync.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/sync.c b/fs/sync.c
index dc725914e1edb..b313db0ebb5ee 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -142,6 +142,7 @@ void emergency_sync(void)
 		schedule_work(work);
 	}
 }
+EXPORT_SYMBOL(emergency_sync);
 
 /*
  * sync a single super
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH] fs: export emergency_sync

[Bill O'Donnell]

On Tue, Jul 18, 2023 at 09:45:40PM +0000, Rob Barnes wrote:
> emergency_sync forces a filesystem sync in emergency situations.
> Export this function so it can be used by modules.
> 
> Signed-off-by: Rob Barnes <robbarnes@google.com>

Example of an emergency situation?
Thanks-
Bill


> ---
> 
>  fs/sync.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/sync.c b/fs/sync.c
> index dc725914e1edb..b313db0ebb5ee 100644
> --- a/fs/sync.c
> +++ b/fs/sync.c
> @@ -142,6 +142,7 @@ void emergency_sync(void)
>  		schedule_work(work);
>  	}
>  }
> +EXPORT_SYMBOL(emergency_sync);
>  
>  /*
>   * sync a single super
> -- 
> 2.41.0.255.g8b1d071c50-goog
> 

Re: [PATCH] fs: export emergency_sync

[Rob Barnes]

My use case is when the EC panics. A hard reset is imminent. In my
testing a regular sync did not always sync all of the logs. See
https://lore.kernel.org/all/20230717232932.1.I361812b405bd07772f66660624188339ab158772@changeid

On Tue, Jul 18, 2023 at 4:13 PM Bill O'Donnell <billodo@redhat.com> wrote:
>
> On Tue, Jul 18, 2023 at 09:45:40PM +0000, Rob Barnes wrote:
> > emergency_sync forces a filesystem sync in emergency situations.
> > Export this function so it can be used by modules.
> >
> > Signed-off-by: Rob Barnes <robbarnes@google.com>
>
> Example of an emergency situation?
> Thanks-
> Bill
>
>
> > ---
> >
> >  fs/sync.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/sync.c b/fs/sync.c
> > index dc725914e1edb..b313db0ebb5ee 100644
> > --- a/fs/sync.c
> > +++ b/fs/sync.c
> > @@ -142,6 +142,7 @@ void emergency_sync(void)
> >               schedule_work(work);
> >       }
> >  }
> > +EXPORT_SYMBOL(emergency_sync);
> >
> >  /*
> >   * sync a single super
> > --
> > 2.41.0.255.g8b1d071c50-goog
> >
>

Re: [PATCH] fs: export emergency_sync

[Guenter Roeck]

On Tue, Jul 18, 2023 at 05:13:06PM -0500, Bill O'Donnell wrote:
> On Tue, Jul 18, 2023 at 09:45:40PM +0000, Rob Barnes wrote:
> > emergency_sync forces a filesystem sync in emergency situations.
> > Export this function so it can be used by modules.
> > 
> > Signed-off-by: Rob Barnes <robbarnes@google.com>
> 
> Example of an emergency situation?

An example from existing code in
drivers/firmware/arm_scmi/scmi_power_control.c:

static inline void
scmi_request_forceful_transition(struct scmi_syspower_conf *sc)
{
        dev_dbg(sc->dev, "Serving forceful request:%d\n",
                sc->required_transition);

#ifndef MODULE
        emergency_sync();
#endif

Arguably emergency_sync() should also be called if the file is built
as module.

Either case, I think it would make sense to add an example to the commit
description.

Thanks,
Guenter

Re: [PATCH] fs: export emergency_sync

[Christian Brauner]

On Tue, Jul 18, 2023 at 09:08:06PM -0700, Guenter Roeck wrote:
> On Tue, Jul 18, 2023 at 05:13:06PM -0500, Bill O'Donnell wrote:
> > On Tue, Jul 18, 2023 at 09:45:40PM +0000, Rob Barnes wrote:
> > > emergency_sync forces a filesystem sync in emergency situations.
> > > Export this function so it can be used by modules.
> > > 
> > > Signed-off-by: Rob Barnes <robbarnes@google.com>
> > 
> > Example of an emergency situation?
> 
> An example from existing code in
> drivers/firmware/arm_scmi/scmi_power_control.c:
> 
> static inline void
> scmi_request_forceful_transition(struct scmi_syspower_conf *sc)
> {
>         dev_dbg(sc->dev, "Serving forceful request:%d\n",
>                 sc->required_transition);
> 
> #ifndef MODULE
>         emergency_sync();
> #endif
> 
> Arguably emergency_sync() should also be called if the file is built
> as module.
> 
> Either case, I think it would make sense to add an example to the commit
> description.

On vacation until next. Please add a proper rationale why and who this
export is needed by in the commit message. As right now it looks like
someone thought it would be good to have which is not enough for
something to become an export.

Re: [PATCH] fs: export emergency_sync

[Christoph Hellwig]

On Wed, Jul 19, 2023 at 07:53:32AM +0200, Christian Brauner wrote:
> On vacation until next. Please add a proper rationale why and who this
> export is needed by in the commit message. As right now it looks like
> someone thought it would be good to have which is not enough for
> something to become an export.

emergency_sync is a relaly bad idea and has all kinds of issues.
It should go away and not grow more users outside of core code,
and the one Guenther points to should never have been added.

If we want to allow emergency shutdowns it needs a proper interface
and not a remount read-only ignoring some rules that tends to make
things worse and instad of better, and even for that I'm not sure
I want modules to be able to drive it.

Re: [PATCH] fs: export emergency_sync

[Dmitry Torokhov]

On Tue, Jul 18, 2023 at 11:31:49PM -0700, Christoph Hellwig wrote:
> On Wed, Jul 19, 2023 at 07:53:32AM +0200, Christian Brauner wrote:
> > On vacation until next. Please add a proper rationale why and who this
> > export is needed by in the commit message. As right now it looks like
> > someone thought it would be good to have which is not enough for
> > something to become an export.
> 
> emergency_sync is a relaly bad idea and has all kinds of issues.
> It should go away and not grow more users outside of core code,
> and the one Guenther points to should never have been added.
> 
> If we want to allow emergency shutdowns it needs a proper interface
> and not a remount read-only ignoring some rules that tends to make
> things worse and instad of better, and even for that I'm not sure
> I want modules to be able to drive it.

I am not sure why you would not want modules to use it - in the case we
have here we detect a catastrophic failure in a critical system
component (embedded controller crashed) and would like to have as much
of the logs saved as possible. It is a module because this kind of EC
may not be present on every system, but when it is present it is very
much a core component.

Thanks.

-- 
Dmitry

Re: [PATCH] fs: export emergency_sync

[Christoph Hellwig]

On Wed, Jul 19, 2023 at 01:51:25PM -0700, Dmitry Torokhov wrote:
> I am not sure why you would not want modules to use it - in the case we
> have here we detect a catastrophic failure in a critical system
> component (embedded controller crashed) and would like to have as much
> of the logs saved as possible. It is a module because this kind of EC
> may not be present on every system, but when it is present it is very
> much a core component.

I really don't think this is a kernel poicy in any way.  Please do
a userspace upcall and let userspace deal with the policy decision.

Re: [PATCH] fs: export emergency_sync

[Guenter Roeck]

On 7/18/23 22:53, Christian Brauner wrote:
> On Tue, Jul 18, 2023 at 09:08:06PM -0700, Guenter Roeck wrote:
>> On Tue, Jul 18, 2023 at 05:13:06PM -0500, Bill O'Donnell wrote:
>>> On Tue, Jul 18, 2023 at 09:45:40PM +0000, Rob Barnes wrote:
>>>> emergency_sync forces a filesystem sync in emergency situations.
>>>> Export this function so it can be used by modules.
>>>>
>>>> Signed-off-by: Rob Barnes <robbarnes@google.com>
>>>
>>> Example of an emergency situation?
>>
>> An example from existing code in
>> drivers/firmware/arm_scmi/scmi_power_control.c:
>>
>> static inline void
>> scmi_request_forceful_transition(struct scmi_syspower_conf *sc)
>> {
>>          dev_dbg(sc->dev, "Serving forceful request:%d\n",
>>                  sc->required_transition);
>>
>> #ifndef MODULE
>>          emergency_sync();
>> #endif
>>
>> Arguably emergency_sync() should also be called if the file is built
>> as module.
>>
>> Either case, I think it would make sense to add an example to the commit
>> description.
> 
> On vacation until next. Please add a proper rationale why and who this
> export is needed by in the commit message. As right now it looks like
> someone thought it would be good to have which is not enough for
> something to become an export.


No, this is just wrong. Did you read Rob's response ? I just pointed out
that there is another user.

Guenter

Re: [PATCH] fs: export emergency_sync

[Christian Brauner]

> that there is another user.

The fact that another non-module user exists is irrelevant to whether or
not this will become an export.