All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: dsterba@suse.cz, David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers
Date: Tue, 27 Jul 2021 10:45:52 +0200	[thread overview]
Message-ID: <20210727084552.GK5047@twin.jikos.cz> (raw)
In-Reply-To: <cc110ee1-c1bf-2b83-b5db-f70468b159f7@gmx.com>

On Tue, Jul 27, 2021 at 06:26:47AM +0800, Qu Wenruo wrote:
> On 2021/7/26 下午11:09, David Sterba wrote:
> > On Mon, Jul 26, 2021 at 08:41:57PM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2021/7/26 下午8:15, David Sterba wrote:
> >>> We have lots of places where we want to obtain inode from page, fs_info
> >>> from page and open code the pointer chains.
> >>
> >> All those inode/fs_info grabbing from just a page is dangerous.
> >>
> >> If an anonymous page is passed in unintentionally, it can easily crash
> >> the system.
> >>
> >> Thus at least some ASSERT() here is a must to me.
> >
> > But we can only check if the pointer is valid, any page can have a valid
> > pointer but not our fs_info. If it crashes on an unexpected page than
> > what can we do in the code anyway?
> 
> What I mean is to check page->mapping for the page passed in.
> 
> Indeed we can't do anything when we hit a page with NULL mapping
> pointer, but that's a code bug.
> An ASSERT() would make us developer aware what's going wrong and to fix
> the bug.

The assert is a more verbose crash, so that's slightly more developer
friendly but I'm still not convinced it's worth the assert. Right now
the macros are not static inlines so they don't need full definitions of
page and mapping and the other types. Embedding the asserts into macros
would look like

  ({ ASSERT(page); ASSERT(page->mapping); page->mapping->host; })

Or perhaps also page with a temporary variable to avoid multiple
evaluations.

The helpers are used in a handful of places, if we really care about
consistency of the assertions, something like assert_page_ok(page) would
have to be in each function that gets the page from other subsystems.

  reply	other threads:[~2021-07-27  8:48 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 12:15 [PATCH 00/10] Misc small cleanups David Sterba
2021-07-26 12:15 ` [PATCH 01/10] btrfs: switch uptodate to bool in btrfs_writepage_endio_finish_ordered David Sterba
2021-07-26 12:23   ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 02/10] btrfs: remove uptodate parameter from btrfs_dec_test_first_ordered_pending David Sterba
2021-07-26 12:24   ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 03/10] btrfs: make btrfs_next_leaf static inline David Sterba
2021-07-26 12:25   ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 04/10] btrfs: tree-checker: use table values for stripe checks David Sterba
2021-07-26 12:29   ` Qu Wenruo
2021-07-26 14:47     ` David Sterba
2021-07-26 12:15 ` [PATCH 05/10] btrfs: tree-checker: add missing stripe checks for raid1c3/4 profiles David Sterba
2021-07-26 12:29   ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 06/10] btrfs: uninline btrfs_bg_flags_to_raid_index David Sterba
2021-07-26 12:34   ` Qu Wenruo
2021-07-26 15:06     ` David Sterba
2021-07-26 12:15 ` [PATCH 07/10] btrfs: merge alloc_device helpers David Sterba
2021-07-26 12:35   ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 08/10] btrfs: simplify data stripe calculation helpers David Sterba
2021-07-26 12:38   ` Qu Wenruo
2021-07-26 15:06     ` David Sterba
2021-07-26 22:23       ` Qu Wenruo
2021-07-27  8:39         ` David Sterba
2021-07-27  9:32           ` Qu Wenruo
2021-07-26 12:15 ` [PATCH 09/10] btrfs: constify and cleanup variables comparators David Sterba
2021-07-26 12:15 ` [PATCH 10/10] btrfs: add and use simple page/bio to inode/fs_info helpers David Sterba
2021-07-26 12:41   ` Qu Wenruo
2021-07-26 15:09     ` David Sterba
2021-07-26 22:26       ` Qu Wenruo
2021-07-27  8:45         ` David Sterba [this message]
2021-07-27  9:42           ` Qu Wenruo
2021-07-27 14:44             ` David Sterba
2021-07-27 15:03   ` [PATCH v2 " David Sterba
2021-07-28  0:12     ` Qu Wenruo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210727084552.GK5047@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=dsterba@suse.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=quwenruo.btrfs@gmx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.