Guys,
why did b5dee3130bb4014 add a magic export for sync functionality
that wrapps VFS code in a weird way, and then exports it (without
even adding a user in that commit)? This kind of functionality
needs to be exported from the VFS, and only with ACKs? With this
and commit d5ea093eebf022e now we end up with a random driver (amdgpu)
syncing all file systems for absolutely no good reason.
On 7/31/2023 9:20 AM, Christoph Hellwig wrote:
> Guys,
>
> why did b5dee3130bb4014 add a magic export for sync functionality
> that wrapps VFS code in a weird way, and then exports it (without
> even adding a user in that commit)? This kind of functionality
> needs to be exported from the VFS, and only with ACKs?
OK, I'll remember about this.
> With this
> and commit d5ea093eebf022e now we end up with a random driver (amdgpu)
> syncing all file systems for absolutely no good reason.
Sorry about that.
The problematic commit should still revert more or less cleanly, so
please do that if that's what you need.
On Tue, Aug 01, 2023 at 04:07:18AM -0700, Christoph Hellwig wrote:
> On Mon, Jul 31, 2023 at 08:27:17PM +0200, Wysocki, Rafael J wrote:
> >
> > OK, I'll remember about this.
> >
> >
> > > With this
> > > and commit d5ea093eebf022e now we end up with a random driver (amdgpu)
> > > syncing all file systems for absolutely no good reason.
> >
> > Sorry about that.
> >
> > The problematic commit should still revert more or less cleanly, so please
> > do that if that's what you need.
>
> We'd still need to remove abuse in amdgpu first, though.
This would effectively revert d5ea093eebf0
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index dc0e5227119b..af04fece37d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -75,7 +75,6 @@
#include "amdgpu_fru_eeprom.h"
#include "amdgpu_reset.h"
-#include <linux/suspend.h>
#include <drm/task_barrier.h>
#include <linux/pm_runtime.h>
@@ -5225,17 +5224,6 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
*/
need_emergency_restart = amdgpu_ras_need_emergency_restart(adev);
- /*
- * Flush RAM to disk so that after reboot
- * the user can read log and see why the system rebooted.
- */
- if (need_emergency_restart && amdgpu_ras_get_context(adev)->reboot) {
- DRM_WARN("Emergency reboot.");
-
- ksys_sync_helper();
- emergency_restart();
- }
-
dev_info(adev->dev, "GPU %s begin!\n",
need_emergency_restart ? "jobs stop":"reset");
On Mon, Jul 31, 2023 at 08:27:17PM +0200, Wysocki, Rafael J wrote:
>
> OK, I'll remember about this.
>
>
> > With this
> > and commit d5ea093eebf022e now we end up with a random driver (amdgpu)
> > syncing all file systems for absolutely no good reason.
>
> Sorry about that.
>
> The problematic commit should still revert more or less cleanly, so please
> do that if that's what you need.
We'd still need to remove abuse in amdgpu first, though.
[Public]
> -----Original Message-----
> From: Matthew Wilcox <[email protected]>
> Sent: Tuesday, August 1, 2023 8:34 AM
> To: Christoph Hellwig <[email protected]>
> Cc: Wysocki, Rafael J <[email protected]>; Christian Brauner
> <[email protected]>; Andrey Grodzovsky <[email protected]>;
> [email protected]; [email protected]; Deucher,
> Alexander <[email protected]>; Zhang, Hawking
> <[email protected]>; Harry Pan <[email protected]>; linux-
> [email protected]
> Subject: Re: ksys_sync_helper
>
> On Tue, Aug 01, 2023 at 04:07:18AM -0700, Christoph Hellwig wrote:
> > On Mon, Jul 31, 2023 at 08:27:17PM +0200, Wysocki, Rafael J wrote:
> > >
> > > OK, I'll remember about this.
> > >
> > >
> > > > With this
> > > > and commit d5ea093eebf022e now we end up with a random driver
> > > > (amdgpu) syncing all file systems for absolutely no good reason.
> > >
> > > Sorry about that.
> > >
> > > The problematic commit should still revert more or less cleanly, so
> > > please do that if that's what you need.
> >
> > We'd still need to remove abuse in amdgpu first, though.
>
> This would effectively revert d5ea093eebf0
>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index dc0e5227119b..af04fece37d5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -75,7 +75,6 @@
> #include "amdgpu_fru_eeprom.h"
> #include "amdgpu_reset.h"
>
> -#include <linux/suspend.h>
> #include <drm/task_barrier.h>
> #include <linux/pm_runtime.h>
>
> @@ -5225,17 +5224,6 @@ int amdgpu_device_gpu_recover(struct
> amdgpu_device *adev,
> */
> need_emergency_restart =
> amdgpu_ras_need_emergency_restart(adev);
>
> - /*
> - * Flush RAM to disk so that after reboot
> - * the user can read log and see why the system rebooted.
> - */
> - if (need_emergency_restart && amdgpu_ras_get_context(adev)-
> >reboot) {
> - DRM_WARN("Emergency reboot.");
> -
> - ksys_sync_helper();
> - emergency_restart();
> - }
> -
Was on PTO last week. I think we can drop this. Will try and send out a patch this week to clean this up.
Alex
> dev_info(adev->dev, "GPU %s begin!\n",
> need_emergency_restart ? "jobs stop":"reset");
>