Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1186315 > unrolled thread

[PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

Started byOleg Nesterov <oleg@redhat.com>
First post2015-07-17 01:20 +0200
Last post2015-07-17 01:30 +0200
Articles 11 — 5 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-17 01:20 +0200
    Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Andrew Morton <akpm@linux-foundation.org> - 2015-07-17 01:30 +0200
      Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-17 02:00 +0200
        Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Benjamin LaHaise <bcrl@kvack.org> - 2015-07-17 16:10 +0200
          Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-18 00:40 +0200
            Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-20 19:40 +0200
              Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Benjamin LaHaise <bcrl@kvack.org> - 2015-07-20 20:00 +0200
                Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-20 20:40 +0200
                Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Jeff Moyer <jmoyer@redhat.com> - 2015-07-20 20:40 +0200
          Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Oleg Nesterov <oleg@redhat.com> - 2015-07-18 01:00 +0200
    Re: [PATCH]  mm-move-mremap-from-file_operations-to-vm_operations_struct-fix Stephen Rothwell <sfr@canb.auug.org.au> - 2015-07-17 01:30 +0200

#1186315 — [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-17 01:20 +0200
Subject[PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix
Message-ID<pN0Bz-4rH-7@gated-at.bofh.it>
fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite()
is not defined in this case. Add yet another "must not be called"
helper into nommu.c to make the linker happy.

I still think this is pointless, afaics sys_io_setup() simply can't
succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend
on CONFIG_MMU.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/nommu.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index e3026fd..979afad 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 EXPORT_SYMBOL(filemap_map_pages);
 
+int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	BUG();
+	return 0;
+}
+
 static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned long addr, void *buf, int len, int write)
 {
-- 
1.5.5.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]


#1186316

FromAndrew Morton <akpm@linux-foundation.org>
Date2015-07-17 01:30 +0200
Message-ID<pN0Lf-4CQ-1@gated-at.bofh.it>
In reply to#1186315
On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <oleg@redhat.com> wrote:

> fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite()
> is not defined in this case. Add yet another "must not be called"
> helper into nommu.c to make the linker happy.
> 
> I still think this is pointless, afaics sys_io_setup() simply can't
> succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend
> on CONFIG_MMU.
> 
> ..
>
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
>  }
>  EXPORT_SYMBOL(filemap_map_pages);
>  
> +int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> +{
> +	BUG();
> +	return 0;
> +}
> +
>  static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
>  		unsigned long addr, void *buf, int len, int write)
>  {

So if anyone starts testing aio on NOMMU, this patch will make the
whole thing immediately go BUG.  This isn't helpful :(

Yes, making AIO depend on MMU sounds better.  Because if it wasn't
busted before, it sure is now!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186335

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-17 02:00 +0200
Message-ID<pN1eh-5aM-11@gated-at.bofh.it>
In reply to#1186316
On 07/16, Andrew Morton wrote:
>
> On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
>
> > fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite()
> > is not defined in this case. Add yet another "must not be called"
> > helper into nommu.c to make the linker happy.
> >
> > I still think this is pointless, afaics sys_io_setup() simply can't
> > succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend
> > on CONFIG_MMU.
> >
> > ..
> >
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
> >  }
> >  EXPORT_SYMBOL(filemap_map_pages);
> >
> > +int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > +{
> > +	BUG();
> > +	return 0;
> > +}
> > +
> >  static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> >  		unsigned long addr, void *buf, int len, int write)
> >  {
>
> So if anyone starts testing aio on NOMMU, this patch will make the
> whole thing immediately go BUG.  This isn't helpful :(

Well, I'm afraid I could miss something, but _afaics_ this can not
happen. filemap_page_mkwrite() can't be called if NOMMU.

In particular, simply because sys_io_setup() is the only user (if
NOMMU) and it can't succeed. But even if I missed something and it
can succeed, ->page_mkwrite() must not be called anyway. But this,
again, unless I missed something ;)

> Yes, making AIO depend on MMU sounds better.

Perhaps Benjamin can change his mind or correct me.

> Because if it wasn't
> busted before, it sure is now!

I hope this change can't make any difference.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186854 — Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

FromBenjamin LaHaise <bcrl@kvack.org>
Date2015-07-17 16:10 +0200
SubjectRe: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix
Message-ID<pNeuR-7FI-13@gated-at.bofh.it>
In reply to#1186335
On Fri, Jul 17, 2015 at 01:52:28AM +0200, Oleg Nesterov wrote:
> On 07/16, Andrew Morton wrote:
> >
> > On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite()
> > > is not defined in this case. Add yet another "must not be called"
> > > helper into nommu.c to make the linker happy.
> > >
> > > I still think this is pointless, afaics sys_io_setup() simply can't
> > > succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend
> > > on CONFIG_MMU.
> > >
> > > ..
> > >
> > > --- a/mm/nommu.c
> > > +++ b/mm/nommu.c
> > > @@ -2008,6 +2008,12 @@ void filemap_map_pages(struct vm_area_struct *vma, struct vm_fault *vmf)
> > >  }
> > >  EXPORT_SYMBOL(filemap_map_pages);
> > >
> > > +int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > +{
> > > +	BUG();
> > > +	return 0;
> > > +}
> > > +
> > >  static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm,
> > >  		unsigned long addr, void *buf, int len, int write)
> > >  {
> >
> > So if anyone starts testing aio on NOMMU, this patch will make the
> > whole thing immediately go BUG.  This isn't helpful :(
> 
> Well, I'm afraid I could miss something, but _afaics_ this can not
> happen. filemap_page_mkwrite() can't be called if NOMMU.
> 
> In particular, simply because sys_io_setup() is the only user (if
> NOMMU) and it can't succeed. But even if I missed something and it
> can succeed, ->page_mkwrite() must not be called anyway. But this,
> again, unless I missed something ;)
> 
> > Yes, making AIO depend on MMU sounds better.
> 
> Perhaps Benjamin can change his mind or correct me.

Either try to fix it correctly, or disable the config.  Making it just 
compile but be knowingly broken is worse than either of those 2 options.  
My point was that it is valid for someone to want to use the functionality 
on a nommu system, and given that it should have worked before the page 
migration code was added, It Would Be Nice(tm) to return it to that state.  
Adding a BUG() like that to the code is just plain broken.

		-ben

> > Because if it wasn't
> > busted before, it sure is now!
> 
> I hope this change can't make any difference.
> 
> Oleg.

-- 
"Thought is the essence of where you are now."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1187256

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-18 00:40 +0200
Message-ID<pNmsq-2cc-21@gated-at.bofh.it>
In reply to#1186854
On 07/17, Benjamin LaHaise wrote:
>
> it should have worked before the page
> migration code was added,

This is off-topic, but the whole "vm" logic in aio_setup_ring()
looks sub-optimal. I do not mean the code, just it seems to me it
is pointless to pollute the page cache, and expose the pages we
can not swap/free to lru. Afaics we _only_ need this for migration.

Perhaps I missed something, doesn't matter. But this means that
this memory is not accounted, so if I increase aio-max-nr then
this test-case

	#define __NR_io_setup	206

	int main(void)
	{
		int nr;

		for (nr = 0; ;++nr) {
			void *ctx = NULL;
			int ret = syscall(__NR_io_setup, 1, &ctx);
			if (ret) {
				printf("failed %d %m: ", nr);
				getchar();
			}
		}

		return 0;
	}

triggers OOM-killer which kills sshd and other daemons on my machine.
These pages were not even faulted in (or the shrinker can unmap them),
the kernel can not know who should be blamed.

Shouldn't we account aio events/pages somehow, say per-user, or in
mm->pinned_vm ?

I do not think this is unkown, and probably this all is fine. IOW,
this is just a question, not a bug-report or something like this.

And of course, this is not exploitable because aio-max-nr limits
the number of pages you can steal.

But otoh, aio_max_nr is system-wide, so the unpriviliged user can
ddos (say) mysqld. And this leads to the same question: shouldn't
we account nr_events at least?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1188391

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-20 19:40 +0200
Message-ID<pOncJ-826-1@gated-at.bofh.it>
In reply to#1187256
Hi Jeff,

On 07/20, Jeff Moyer wrote:
>
> Hi, Oleg,
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Shouldn't we account aio events/pages somehow, say per-user, or in
> > mm->pinned_vm ?
>
> Ages ago I wrote a patch to account the completion ring to a process'
> memlock limit:
>   "[patch] aio: remove aio-max-nr and instead use the memlock rlimit to
>    limit the number of pages pinned for the aio completion ring"
>   http://marc.info/?l=linux-aio&m=123661380807041&w=2
>
> The problem with that patch is that it modifies the user/kernel
> interface.  It could be done over time, as Andrew outlined in that
> thread, but I've been reluctant to take that on.

See also the usage of mm->pinned_vm and user->locked_vm in perf_mmap(),
perhaps aio can do the same...

> If you just mean we should account the memory so that the right process
> can be killed, that sounds like a good idea to me.

Not sure we actually need this. I only meant that this looks confusing
because this memory is actually locked but the kernel doesn't know this.

And btw, I forgot to mention that I triggered OOM on the testing machine
with only 512mb ram, and aio-max-nr was huge. So, once again, while this
all doesn't look right to me, I do not think this is the real problem.

Except the fact that an unpriviliged user can steal all aio-max-nr events.
This probably worth fixing in any case.



And if we accept the fact this memory is locked and if we properly account
it, then may be we can just kill aio_migratepage(), aio_private_file(), and
change aio_setup_ring() to simply use install_special_mapping(). This will
greatly simplify the code. But let me remind that I know nothing about aio,
so please don't take my thoughts seriously.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1188410 — Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

FromBenjamin LaHaise <bcrl@kvack.org>
Date2015-07-20 20:00 +0200
SubjectRe: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix
Message-ID<pOnw6-8oH-29@gated-at.bofh.it>
In reply to#1188391
On Mon, Jul 20, 2015 at 07:33:11PM +0200, Oleg Nesterov wrote:
> Hi Jeff,
> 
> On 07/20, Jeff Moyer wrote:
> >
> > Hi, Oleg,
> >
> > Oleg Nesterov <oleg@redhat.com> writes:
> >
> > > Shouldn't we account aio events/pages somehow, say per-user, or in
> > > mm->pinned_vm ?
> >
> > Ages ago I wrote a patch to account the completion ring to a process'
> > memlock limit:
> >   "[patch] aio: remove aio-max-nr and instead use the memlock rlimit to
> >    limit the number of pages pinned for the aio completion ring"
> >   http://marc.info/?l=linux-aio&m=123661380807041&w=2
> >
> > The problem with that patch is that it modifies the user/kernel
> > interface.  It could be done over time, as Andrew outlined in that
> > thread, but I've been reluctant to take that on.
> 
> See also the usage of mm->pinned_vm and user->locked_vm in perf_mmap(),
> perhaps aio can do the same...
> 
> > If you just mean we should account the memory so that the right process
> > can be killed, that sounds like a good idea to me.
> 
> Not sure we actually need this. I only meant that this looks confusing
> because this memory is actually locked but the kernel doesn't know this.
> 
> And btw, I forgot to mention that I triggered OOM on the testing machine
> with only 512mb ram, and aio-max-nr was huge. So, once again, while this
> all doesn't look right to me, I do not think this is the real problem.
> 
> Except the fact that an unpriviliged user can steal all aio-max-nr events.
> This probably worth fixing in any case.
> 
> 
> 
> And if we accept the fact this memory is locked and if we properly account
> it, then may be we can just kill aio_migratepage(), aio_private_file(), and
> change aio_setup_ring() to simply use install_special_mapping(). This will
> greatly simplify the code. But let me remind that I know nothing about aio,
> so please don't take my thoughts seriously.

No, you can't get rid of that code.  The page migration is required when 
CPUs/memory is offlined and data needs to be moved to another node.  
Similarly, support for mremap() is also required for container migration / 
restoration.

As for accounting locked memory, we don't do that for memory pinned by 
O_DIRECT either.  Given how small the amount of memory aio can pin is 
compared to O_DIRECT or mlock(), it is unlikely that the accounting of 
how much aio has pinned will make any real difference in the big picture.  
A single O_DIRECT i/o can pin megabytes of memory.

		-ben

> Oleg.

-- 
"Thought is the essence of where you are now."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1188426

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-20 20:40 +0200
Message-ID<pOo8N-Vo-1@gated-at.bofh.it>
In reply to#1188410
On 07/20, Benjamin LaHaise wrote:
>
> On Mon, Jul 20, 2015 at 07:33:11PM +0200, Oleg Nesterov wrote:
> >
> > And if we accept the fact this memory is locked and if we properly account
> > it, then may be we can just kill aio_migratepage(), aio_private_file(), and
> > change aio_setup_ring() to simply use install_special_mapping(). This will
> > greatly simplify the code. But let me remind that I know nothing about aio,
> > so please don't take my thoughts seriously.
>
> No, you can't get rid of that code.  The page migration is required when
> CPUs/memory is offlined and data needs to be moved to another node.

Of course, if we remove aio_migratepage() then aio can't be moved,

> Similarly, support for mremap() is also required for container migration /
> restoration.

This is not the problem. And one of the reasons to move ->mremap() into
vm_operations_struct was that install_special_mapping() can use it.

> Given how small the amount of memory aio can pin

I agree, but why should we worry about migration then? let this memory be
unmovable, don't use GFP_RECLAIMABLE/MOVABLE, etc.

But again, again, please ignore. This all is off-topic and my understanding
is very limited.

> it is unlikely that the accounting of
> how much aio has pinned will make any real difference in the big picture.

Agreed, but this can help to remove the system-wide aio-max-nr. Again,
unpriviliged user can steal aio.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1188432 — Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix

FromJeff Moyer <jmoyer@redhat.com>
Date2015-07-20 20:40 +0200
SubjectRe: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix
Message-ID<pOo8O-Vo-15@gated-at.bofh.it>
In reply to#1188410
Benjamin LaHaise <bcrl@kvack.org> writes:

> As for accounting locked memory, we don't do that for memory pinned by 
> O_DIRECT either.  Given how small the amount of memory aio can pin is 
> compared to O_DIRECT or mlock(), it is unlikely that the accounting of 
> how much aio has pinned will make any real difference in the big picture.  
> A single O_DIRECT i/o can pin megabytes of memory.

Actually, you can pin a lot of memory with aio.  Worst case, aio-max-nr
represents the maximum number of pages you can lock in memory (assuming
you pass 1 to io_setup in a loop).  So, for the default of 65536 events,
that comes to 256MB.

My system has libvirt installed, which changes the default to 1 million:
$ grep aio-max-nr /usr/lib/sysctl.d/libvirtd.conf
fs.aio-max-nr = 1048576

So, that means up to 4GB of memory can be tied up by aio.  Oracle's
installation guide recommends the same.

The difference between the aio ring and something like DIO is that the
ring is long-lived.  I/O, ideally, doesn't take that long, so that's a
poor comparison to make.

I tend to agree with Oleg, a system-wide setting just isn't a good fit
for this.  Changing it now that it's in place is difficult, though, and
obviously low priority.

-Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1187269

FromOleg Nesterov <oleg@redhat.com>
Date2015-07-18 01:00 +0200
Message-ID<pNmLL-2yN-1@gated-at.bofh.it>
In reply to#1186854
Benjamin,

This discussion was a bit confusing, so let me try to summarize and
please correct me if I misunderstood.

And sorry for bothering you, I just want to fix and forget about the
problem which was introduced by me (build failure with NOMMU).

So _iiuc_ you are starting to agree that tehcnically this change is
correct and we can never hit this BUG(). Like we can never hit another
BUG() in nommu.c:filemap_fault(), also referenced by aio_ring_vm_ops.
And this change should not hurt even if you make aio work with NOMMU.

No?

However, you still dislike this change because you think it is sub-
optimal and/or not clean enough. I won't argue with maintainer.

So what do you suggest instead? Will you agree with ifdef(CONFIG_MMU)
in aio_ring_vm_ops?

I aggree in advance with any suggestion.

On 07/17, Oleg Nesterov wrote:
>
> On 07/17, Benjamin LaHaise wrote:
> >
> > On Fri, Jul 17, 2015 at 07:27:26PM +0200, Oleg Nesterov wrote:
> > > Benjamin,
> > >
> > > it seems that we do not understand each other,
> > ...
> > > >
> > > > Either try to fix it correctly,
> > >
> > > And I think this fix is correct. In a sense that we only add
> > > filemap_page_mkwrite() to make the linker happy, it can never be called
> > > and thus we can never hit this BUG().
> > >
> > > Please look at filemap_fault() in nommu.c,
> > >
> > > 	int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > > 	{
> > > 		BUG();
> > > 		return 0;
> > > 	}
> > >
> > > this is the same thing. If nothing else, mm/memory.c is not even compiled
> > > if NOMMU.
> >
> > Using BUG() is the wrong approach.  If the code is not needed in NOMMU, then
> > #ifdef it out.  Think about it: NOMMU systems are very low memory systems
> > and they should not have dead code compiled in if it is not needed.
>
> OK, at least I hope you no longer think that this patch makes this code
> knowingly broken.
>
> > Don't add BUG().  It's the equivalent approach of saying "I think this code
> > isn't needed, but I'm lazy and not going to remove it properly."
>
> There is another interpretation: I think this code must be never called,
> if it is actually called we have a serious problem which should be loudly
> reported.
>
> > > Why? Could you explain what I have missed?
> >
> > It's doing half the job.  Either the code should be #if'd out or not.
>
> Again, filemap_page_mkwrite() added to nommu.c matches filemap_fault()
> and filemap_map_pages() we already have.
>
>
> But I won't argue, you are maintainer. What exactly do you want me to
> ifdef? Will you agree with the patch which adds ifdef into
> aio_ring_vm_ops,
>
> 	static const struct vm_operations_struct aio_ring_vm_ops = {
> 	       .mremap         = aio_ring_mremap,
> 	#ifdef CONFIG_MMU
> 	       .fault          = filemap_fault,
> 	       .map_pages      = filemap_map_pages,
> 	       .page_mkwrite   = filemap_page_mkwrite,
> 	#endif
> 	};
>
> ?
>
> Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1186319

FromStephen Rothwell <sfr@canb.auug.org.au>
Date2015-07-17 01:30 +0200
Message-ID<pN0Lg-4CQ-9@gated-at.bofh.it>
In reply to#1186315
Hi Oleg,

On Fri, 17 Jul 2015 01:14:05 +0200 Oleg Nesterov <oleg@redhat.com> wrote:
>
> fs/aio.c can't be compiled if CONFIG_MMU=n, filemap_page_mkwrite()
> is not defined in this case. Add yet another "must not be called"
> helper into nommu.c to make the linker happy.
> 
> I still think this is pointless, afaics sys_io_setup() simply can't
> succeed if CONFIG_MMU=n. Instead we should make CONFIG_AIO depend
> on CONFIG_MMU.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Added as a fix to linux-next today.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web