Re: ext4 corruption on alpha with 4.20.0-09062-gd8372ba8ce28
On Thu 21-02-19 01:23:50, Meelis Roos wrote: > > > First, I found out that both the problematic alphas had memory compaction > > > and > > > page migration and bounce buffers turned on, and working alphas had them > > > off. > > > > > > Next, turing off these options makes the problematic alphas work. > > > > OK, thanks for testing! Can you narrow down whether the problem is due to > > CONFIG_BOUNCE or CONFIG_MIGRATION + CONFIG_COMPACTION? These are two > > completely different things so knowing where to look will help. Thanks! > > Tested both. > > Just CONFIG_MIGRATION + CONFIG_COMPACTION breaks the alpha. > Just CONFIG_BOUNCE has no effect in 5 tries. OK, so page migration is problematic. Thanks for confirmation! Honza -- Jan Kara SUSE Labs, CR
Re: ext4 corruption on alpha with 4.20.0-09062-gd8372ba8ce28
On Wed 20-02-19 08:31:05, Meelis Roos wrote:
> > Could
> > https://lore.kernel.org/linux-mm/20190219123212.29838-1-lar...@axis.com/T/#u
> > be relevant?
>
> Tried it, still broken.
OK, I didn't put too much hope into this patch as you see filesystem
metadata corruption so icache/dcache coherency issues seemed unlikely.
Still good that you've tried so that we are sure.
> I wrote:
>
> > But my kernel config had memory compaction (that turned on page migration)
> > and
> > bounce buffers. I do not remember why I found them necessary but I will try
> > without them.
>
> First, I found out that both the problematic alphas had memory compaction and
> page migration and bounce buffers turned on, and working alphas had them off.
>
> Next, turing off these options makes the problematic alphas work.
OK, thanks for testing! Can you narrow down whether the problem is due to
CONFIG_BOUNCE or CONFIG_MIGRATION + CONFIG_COMPACTION? These are two
completely different things so knowing where to look will help. Thanks!
Honza
--
Jan Kara
SUSE Labs, CR
Re: ext4 corruption on alpha with 4.20.0-09062-gd8372ba8ce28
On Tue 19-02-19 14:17:09, Meelis Roos wrote: > > > > > The result of the bisection is > > > > > [88dbcbb3a4847f5e6dfeae952d3105497700c128] blkdev: avoid migration > > > > > stalls for blkdev pages > > > > > > > > > > Is that result relevant for the problem or should I continue > > > > > bisecting between 4.20.0 and the so far first bad commit? > > > > > > > > Can you try reverting the commit and see if it makes the problem go > > > > away? > > > > > > Tried reverting it on top of 5.0.0-rc6-00153-g5ded5871030e and it seems > > > to make the kernel work - emerge --sync succeeded. > There is more to it. > > After running 5.0.0-rc6-00153-g5ded5871030e-dirty (with the revert of > that patch) successfully for Gentoo update, I upgraded the kernel to > 5.0.0-rc7-00011-gb5372fe5dc84-dirty (todays git + revert of this patch) > and it broke on rsync again: > > RepoStorageException: command exited with status -6: rsync -a --link-dest > /usr/portage --exclude=/distfiles --exclude=/local --exclude=/lost+found > --exclude=/packages --exclude /.tmp-unverified-download-quarantine > /usr/portage/ /usr/portage/.tmp-unverified-download-quarantine/ > > Nothing in dmesg. > > This means the real root reason is somewhere deeper and reverting this > commit just made it less likely to happen. Thanks for information. Yeah, that makes somewhat more sense. Can you ever see the failure if you disable CONFIG_TRANSPARENT_HUGEPAGE? Because your findings still seem to indicate that there' some problem with page migration and Alpha (added MM list to CC). Honza -- Jan Kara SUSE Labs, CR
Re: ext4 corruption on alpha with 4.20.0-09062-gd8372ba8ce28
On Sun 17-02-19 00:29:40, Meelis Roos wrote: > > > The result of the bisection is > > > [88dbcbb3a4847f5e6dfeae952d3105497700c128] blkdev: avoid migration stalls > > > for blkdev pages > > > > > > Is that result relevant for the problem or should I continue bisecting > > > between 4.20.0 and the so far first bad commit? > > > > Can you try reverting the commit and see if it makes the problem go away? > > Tried reverting it on top of 5.0.0-rc6-00153-g5ded5871030e and it seems > to make the kernel work - emerge --sync succeeded. > > Unfinished further bisection has also not yielded any other bad revisions > so far. Hum, weird. I have hard time understanding how that change could be causing fs corruption on Aplha but OTOH it is not completely unthinkable. With this commit we may migrate some block device pages we were not able to migrate previously and that could be causing some unexpected issue. I'll look into this. Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH 05/10] mm: replace get_vaddr_frames() write/force parameters with gup_flags
On Thu 13-10-16 01:20:15, Lorenzo Stoakes wrote:
> This patch removes the write and force parameters from get_vaddr_frames() and
> replaces them with a gup_flags parameter to make the use of FOLL_FORCE
> explicit
> in callers as use of this flag can result in surprising behaviour (and hence
> bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
Looks good. You can add:
Reviewed-by: Jan Kara <j...@suse.cz>
Honza
> ---
> drivers/gpu/drm/exynos/exynos_drm_g2d.c| 3 ++-
> drivers/media/platform/omap/omap_vout.c| 2 +-
> drivers/media/v4l2-core/videobuf2-memops.c | 6 +-
> include/linux/mm.h | 2 +-
> mm/frame_vector.c | 13 ++---
> 5 files changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index aa92dec..fbd13fa 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -488,7 +488,8 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct
> drm_device *drm_dev,
> goto err_free;
> }
>
> - ret = get_vaddr_frames(start, npages, true, true, g2d_userptr->vec);
> + ret = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE,
> + g2d_userptr->vec);
> if (ret != npages) {
> DRM_ERROR("failed to get user pages from userptr.\n");
> if (ret < 0)
> diff --git a/drivers/media/platform/omap/omap_vout.c
> b/drivers/media/platform/omap/omap_vout.c
> index e668dde..a31b95c 100644
> --- a/drivers/media/platform/omap/omap_vout.c
> +++ b/drivers/media/platform/omap/omap_vout.c
> @@ -214,7 +214,7 @@ static int omap_vout_get_userptr(struct videobuf_buffer
> *vb, u32 virtp,
> if (!vec)
> return -ENOMEM;
>
> - ret = get_vaddr_frames(virtp, 1, true, false, vec);
> + ret = get_vaddr_frames(virtp, 1, FOLL_WRITE, vec);
> if (ret != 1) {
> frame_vector_destroy(vec);
> return -EINVAL;
> diff --git a/drivers/media/v4l2-core/videobuf2-memops.c
> b/drivers/media/v4l2-core/videobuf2-memops.c
> index 3c3b517..1cd322e 100644
> --- a/drivers/media/v4l2-core/videobuf2-memops.c
> +++ b/drivers/media/v4l2-core/videobuf2-memops.c
> @@ -42,6 +42,10 @@ struct frame_vector *vb2_create_framevec(unsigned long
> start,
> unsigned long first, last;
> unsigned long nr;
> struct frame_vector *vec;
> + unsigned int flags = FOLL_FORCE;
> +
> + if (write)
> + flags |= FOLL_WRITE;
>
> first = start >> PAGE_SHIFT;
> last = (start + length - 1) >> PAGE_SHIFT;
> @@ -49,7 +53,7 @@ struct frame_vector *vb2_create_framevec(unsigned long
> start,
> vec = frame_vector_create(nr);
> if (!vec)
> return ERR_PTR(-ENOMEM);
> - ret = get_vaddr_frames(start & PAGE_MASK, nr, write, true, vec);
> + ret = get_vaddr_frames(start & PAGE_MASK, nr, flags, vec);
> if (ret < 0)
> goto out_destroy;
> /* We accept only complete set of PFNs */
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 27ab538..5ff084f6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1305,7 +1305,7 @@ struct frame_vector {
> struct frame_vector *frame_vector_create(unsigned int nr_frames);
> void frame_vector_destroy(struct frame_vector *vec);
> int get_vaddr_frames(unsigned long start, unsigned int nr_pfns,
> - bool write, bool force, struct frame_vector *vec);
> + unsigned int gup_flags, struct frame_vector *vec);
> void put_vaddr_frames(struct frame_vector *vec);
> int frame_vector_to_pages(struct frame_vector *vec);
> void frame_vector_to_pfns(struct frame_vector *vec);
> diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> index 81b6749..db77dcb 100644
> --- a/mm/frame_vector.c
> +++ b/mm/frame_vector.c
> @@ -11,10 +11,7 @@
> * get_vaddr_frames() - map virtual addresses to pfns
> * @start: starting user address
> * @nr_frames: number of pages / pfns from start to map
> - * @write: whether pages will be written to by the caller
> - * @force: whether to force write access even if user mapping is
> - * readonly. See description of the same argument of
> - get_user_pages().
> + * @gup_flags: flags modifying lookup behaviour
> * @vec: structure which receives pages / pfns of the addresses mapped.
> * It should have space for at least nr_frames entries.
&g
Re: [PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
On Thu 13-10-16 01:20:18, Lorenzo Stoakes wrote:
> This patch removes the write parameter from __access_remote_vm() and replaces
> it
> with a gup_flags parameter as use of this function previously _implied_
> FOLL_FORCE, whereas after this patch callers explicitly pass this flag.
>
> We make this explicit as use of FOLL_FORCE can result in surprising behaviour
> (and hence bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
So I'm not convinced this (and the following two patches) is actually
helping much. By grepping for FOLL_FORCE we will easily see that any caller
of access_remote_vm() gets that semantics and can thus continue search
accordingly (it is much simpler than searching for all get_user_pages()
users and extracting from parameter lists what they actually pass as
'force' argument). Sure it makes somewhat more visible to callers of
access_remote_vm() that they get FOLL_FORCE semantics but OTOH it also
opens a space for issues where a caller of access_remote_vm() actually
wants FOLL_FORCE (and currently all of them want it) and just mistakenly
does not set it. All in all I'd prefer to keep access_remote_vm() and
friends as is...
Honza
> ---
> mm/memory.c | 23 +++
> mm/nommu.c | 9 ++---
> 2 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 20a9adb..79ebed3 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3869,14 +3869,11 @@ EXPORT_SYMBOL_GPL(generic_access_phys);
> * given task for page fault accounting.
> */
> static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> - unsigned long addr, void *buf, int len, int write)
> + unsigned long addr, void *buf, int len, unsigned int gup_flags)
> {
> struct vm_area_struct *vma;
> void *old_buf = buf;
> - unsigned int flags = FOLL_FORCE;
> -
> - if (write)
> - flags |= FOLL_WRITE;
> + int write = gup_flags & FOLL_WRITE;
>
> down_read(>mmap_sem);
> /* ignore errors, just check how much was successfully transferred */
> @@ -3886,7 +3883,7 @@ static int __access_remote_vm(struct task_struct *tsk,
> struct mm_struct *mm,
> struct page *page = NULL;
>
> ret = get_user_pages_remote(tsk, mm, addr, 1,
> - flags, , );
> + gup_flags, , );
> if (ret <= 0) {
> #ifndef CONFIG_HAVE_IOREMAP_PROT
> break;
> @@ -3945,7 +3942,12 @@ static int __access_remote_vm(struct task_struct *tsk,
> struct mm_struct *mm,
> int access_remote_vm(struct mm_struct *mm, unsigned long addr,
> void *buf, int len, int write)
> {
> - return __access_remote_vm(NULL, mm, addr, buf, len, write);
> + unsigned int flags = FOLL_FORCE;
> +
> + if (write)
> + flags |= FOLL_WRITE;
> +
> + return __access_remote_vm(NULL, mm, addr, buf, len, flags);
> }
>
> /*
> @@ -3958,12 +3960,17 @@ int access_process_vm(struct task_struct *tsk,
> unsigned long addr,
> {
> struct mm_struct *mm;
> int ret;
> + unsigned int flags = FOLL_FORCE;
>
> mm = get_task_mm(tsk);
> if (!mm)
> return 0;
>
> - ret = __access_remote_vm(tsk, mm, addr, buf, len, write);
> + if (write)
> + flags |= FOLL_WRITE;
> +
> + ret = __access_remote_vm(tsk, mm, addr, buf, len, flags);
> +
> mmput(mm);
>
> return ret;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 70cb844..bde7df3 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1809,9 +1809,10 @@ void filemap_map_pages(struct fault_env *fe,
> EXPORT_SYMBOL(filemap_map_pages);
>
> static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> - unsigned long addr, void *buf, int len, int write)
> + unsigned long addr, void *buf, int len, unsigned int gup_flags)
> {
> struct vm_area_struct *vma;
> + int write = gup_flags & FOLL_WRITE;
>
> down_read(>mmap_sem);
>
> @@ -1853,7 +1854,8 @@ static int __access_remote_vm(struct task_struct *tsk,
> struct mm_struct *mm,
> int access_remote_vm(struct mm_struct *mm, unsigned long addr,
> void *buf, int len, int write)
> {
> - return __access_remote_vm(NULL, mm, addr, buf, len, write);
> + return __access_remote_vm(NULL, mm, addr, buf, len,
> + write ? FOLL_WRITE : 0);
> }
>
> /*
> @@ -1871,7 +1873,8 @@ int access_process_vm(struct task_struct *tsk, unsigned
>
Re: [PATCH 06/10] mm: replace get_user_pages() write/force parameters with gup_flags
On Thu 13-10-16 01:20:16, Lorenzo Stoakes wrote:
> This patch removes the write and force parameters from get_user_pages() and
> replaces them with a gup_flags parameter to make the use of FOLL_FORCE
> explicit
> in callers as use of this flag can result in surprising behaviour (and hence
> bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
The patch looks good. You can add:
Reviewed-by: Jan Kara <j...@suse.cz>
Honza
> ---
> arch/cris/arch-v32/drivers/cryptocop.c | 4 +---
> arch/ia64/kernel/err_inject.c | 2 +-
> arch/x86/mm/mpx.c | 5 ++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 7 +--
> drivers/gpu/drm/radeon/radeon_ttm.c| 3 ++-
> drivers/gpu/drm/via/via_dmablit.c | 4 ++--
> drivers/infiniband/core/umem.c | 6 +-
> drivers/infiniband/hw/mthca/mthca_memfree.c| 2 +-
> drivers/infiniband/hw/qib/qib_user_pages.c | 3 ++-
> drivers/infiniband/hw/usnic/usnic_uiom.c | 5 -
> drivers/media/v4l2-core/videobuf-dma-sg.c | 7 +--
> drivers/misc/mic/scif/scif_rma.c | 3 +--
> drivers/misc/sgi-gru/grufault.c| 2 +-
> drivers/platform/goldfish/goldfish_pipe.c | 3 ++-
> drivers/rapidio/devices/rio_mport_cdev.c | 3 ++-
> .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 3 +--
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +--
> drivers/virt/fsl_hypervisor.c | 4 ++--
> include/linux/mm.h | 2 +-
> mm/gup.c | 12 +++-
> mm/mempolicy.c | 2 +-
> mm/nommu.c | 18
> --
> 22 files changed, 49 insertions(+), 54 deletions(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c
> b/arch/cris/arch-v32/drivers/cryptocop.c
> index b5698c8..099e170 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -2722,7 +2722,6 @@ static int cryptocop_ioctl_process(struct inode *inode,
> struct file *filp, unsig
> err = get_user_pages((unsigned long int)(oper.indata + prev_ix),
>noinpages,
>0, /* read access only for in data */
> - 0, /* no force */
>inpages,
>NULL);
>
> @@ -2736,8 +2735,7 @@ static int cryptocop_ioctl_process(struct inode *inode,
> struct file *filp, unsig
> if (oper.do_cipher){
> err = get_user_pages((unsigned long int)oper.cipher_outdata,
>nooutpages,
> - 1, /* write access for out data */
> - 0, /* no force */
> + FOLL_WRITE, /* write access for out data */
>outpages,
>NULL);
> up_read(>mm->mmap_sem);
> diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c
> index 09f8457..5ed0ea9 100644
> --- a/arch/ia64/kernel/err_inject.c
> +++ b/arch/ia64/kernel/err_inject.c
> @@ -142,7 +142,7 @@ store_virtual_to_phys(struct device *dev, struct
> device_attribute *attr,
> u64 virt_addr=simple_strtoull(buf, NULL, 16);
> int ret;
>
> - ret = get_user_pages(virt_addr, 1, VM_READ, 0, NULL, NULL);
> + ret = get_user_pages(virt_addr, 1, FOLL_WRITE, NULL, NULL);
> if (ret<=0) {
> #ifdef ERR_INJ_DEBUG
> printk("Virtual address %lx is not existing.\n",virt_addr);
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index 8047687..e4f8009 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -544,10 +544,9 @@ static int mpx_resolve_fault(long __user *addr, int
> write)
> {
> long gup_ret;
> int nr_pages = 1;
> - int force = 0;
>
> - gup_ret = get_user_pages((unsigned long)addr, nr_pages, write,
> - force, NULL, NULL);
> + gup_ret = get_user_pages((unsigned long)addr, nr_pages,
> + write ? FOLL_WRITE : 0, NULL, NULL);
> /*
>* get_user_pages() returns number of pages gotten.
>* 0 means we failed to fault in and get anything,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdg
Re: [PATCH 04/10] mm: replace get_user_pages_locked() write/force parameters with gup_flags
On Tue 18-10-16 14:56:09, Lorenzo Stoakes wrote: > On Tue, Oct 18, 2016 at 02:54:25PM +0200, Jan Kara wrote: > > > @@ -1282,7 +1282,7 @@ long get_user_pages(unsigned long start, unsigned > > > long nr_pages, > > > int write, int force, struct page **pages, > > > struct vm_area_struct **vmas); > > > long get_user_pages_locked(unsigned long start, unsigned long nr_pages, > > > - int write, int force, struct page **pages, int *locked); > > > + unsigned int gup_flags, struct page **pages, int *locked); > > > > Hum, the prototype is inconsistent with e.g. __get_user_pages_unlocked() > > where gup_flags come after **pages argument. Actually it makes more sense > > to have it before **pages so that input arguments come first and output > > arguments second but I don't care that much. But it definitely should be > > consistent... > > It was difficult to decide quite how to arrange parameters as there was > inconsitency with regards to parameter ordering already - for example > __get_user_pages() places its flags argument before pages whereas, as you > note, > __get_user_pages_unlocked() puts them afterwards. > > I ended up compromising by trying to match the existing ordering of the > function > as much as I could by replacing write, force pairs with gup_flags in the same > location (with the exception of get_user_pages_unlocked() which I felt should > match __get_user_pages_unlocked() in signature) or if there was already a > gup_flags parameter as in the case of __get_user_pages_unlocked() I simply > removed the write, force pair and left the flags as the last parameter. > > I am happy to rearrange parameters as needed, however I am not sure if it'd be > worthwhile for me to do so (I am keen to try to avoid adding too much noise > here > :) > > If we were to rearrange parameters for consistency I'd suggest adjusting > __get_user_pages_unlocked() to put gup_flags before pages and do the same with > get_user_pages_unlocked(), let me know what you think. Yeah, ok. If the inconsistency is already there, just leave it for now. Honza -- Jan Kara <j...@suse.com> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()
On Thu 13-10-16 01:20:12, Lorenzo Stoakes wrote:
> This patch removes the write and force parameters from
> __get_user_pages_unlocked() to make the use of FOLL_FORCE explicit in callers
> as
> use of this flag can result in surprising behaviour (and hence bugs) within
> the
> mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
The patch looks good. You can add:
Reviewed-by: Jan Kara <j...@suse.cz>
Honza
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/10] mm: remove write/force parameters from __get_user_pages_locked()
On Thu 13-10-16 01:20:11, Lorenzo Stoakes wrote:
> This patch removes the write and force parameters from
> __get_user_pages_locked()
> to make the use of FOLL_FORCE explicit in callers as use of this flag can
> result
> in surprising behaviour (and hence bugs) within the mm subsystem.
>
> Signed-off-by: Lorenzo Stoakes <lstoa...@gmail.com>
Looks good. You can add:
Reviewed-by: Jan Kara <j...@suse.cz>
Honza
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
