From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Enberg Subject: Re: [rfc][patch] mm, fs: warn on missing address space operations Date: Mon, 22 Mar 2010 10:00:54 +0200 Message-ID: <84144f021003220100r29ee1ff2x11a66531e0104167@mail.gmail.com> References: <20100322053937.GA17637@laptop> <20100322005610.5dfa70b1.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Nick Piggin , linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org To: Andrew Morton Return-path: In-Reply-To: <20100322005610.5dfa70b1.akpm@linux-foundation.org> Sender: owner-linux-mm@kvack.org List-Id: linux-fsdevel.vger.kernel.org On Mon, Mar 22, 2010 at 6:56 AM, Andrew Morton wrote: > On Mon, 22 Mar 2010 16:39:37 +1100 Nick Piggin wrote: > >> It's ugly and lazy that we do these default aops in case it has not >> been filled in by the filesystem. >> >> A NULL operation should always mean either: we don't support the >> operation; we don't require any action; or a bug in the filesystem, >> depending on the context. >> >> In practice, if we get rid of these fallbacks, it will be clearer >> what operations are used by a given address_space_operations struct, >> reduce branches, reduce #if BLOCK ifdefs, and should allow us to get >> rid of all the buffer_head knowledge from core mm and fs code. > > I guess this is one way of waking people up. > > What happens is that hundreds of bug reports land in my inbox and I get > to route them to various maintainers, most of whom don't exist, so > warnings keep on landing in my inbox. =A0Please send a mailing address fo= r > my invoices. > > It would be more practical, more successful and quicker to hunt down > the miscreants and send them rude emails. =A0Plus it would save you > money. > >> We could add a patch like this which spits out a recipe for how to fix >> up filesystems and get them all converted quite easily. >> >> ... >> >> @@ -40,8 +40,14 @@ void do_invalidatepage(struct page *page >> =A0 =A0 =A0 void (*invalidatepage)(struct page *, unsigned long); >> =A0 =A0 =A0 invalidatepage =3D page->mapping->a_ops->invalidatepage; >> =A0#ifdef CONFIG_BLOCK >> - =A0 =A0 if (!invalidatepage) >> + =A0 =A0 if (!invalidatepage) { >> + =A0 =A0 =A0 =A0 =A0 =A0 static bool warned =3D false; >> + =A0 =A0 =A0 =A0 =A0 =A0 if (!warned) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 warned =3D true; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 print_symbol("address_space_op= erations %s missing invalidatepage method. Use block_invalidatepage.\n", (u= nsigned long)page->mapping->a_ops); >> + =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 invalidatepage =3D block_invalidatepage; >> + =A0 =A0 } > > erk, I realise 80 cols can be a pain, but 165 cols is just out of > bounds. =A0Why not > > =A0 =A0 =A0 =A0/* this fs should use block_invalidatepage() */ > =A0 =A0 =A0 =A0WARN_ON_ONCE(!invalidatepage); /me gets his paint bucket... How about WARN_ONCE(!invalidatepage, "this fs should use block_invalidatepage()") Pekka -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org