linux-ext4 - Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87inmi15md.fsf@notabene.neil.brown.name>
Date:   Thu, 06 Apr 2017 15:12:42 +1000
From:   NeilBrown <neilb@...e.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Jeff Layton <jlayton@...hat.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
        akpm@...ux-foundation.org, tytso@....edu, jack@...e.cz
Subject: Re: [RFC PATCH 0/4] fs: introduce new writeback error tracking infrastructure and convert ext4 to use it

On Wed, Apr 05 2017, Matthew Wilcox wrote:

> On Thu, Apr 06, 2017 at 10:02:48AM +1000, NeilBrown wrote:
>> If you are concerned about space in 'struct address_space', just prune
>> some wastage.
>
> I'm trying to (via wlists).  still buggy though.

Cool.
(I wonder what a wlist is.... weighted list?)

>
>> The "host" field brings no value.  It is only ever assigned in
>> inode_init_always():
>> 
>> 	struct address_space *const mapping = &inode->i_data;
>> ......
>> 	mapping->host = inode;
>> 
>> So you could change all references to use
>>    container_of(mapping, struct inode, i_data)
>
> Alas, no:
>
> drivers/dax/dax.c:      inode->i_mapping->host = dax_dev->inode;

	inode->i_mapping = dax_dev->inode->i_mapping;
	inode->i_mapping->host = dax_dev->inode;
so that second line is equivalent to
        dax_dev->inode->i_mapping->host = dax_dev->inode;
so inode->mapping->host leads back to inode.  So this doesn't break the
invariant.

> fs/gfs2/glock.c:                mapping->host = s->s_bdev->bd_inode;
> fs/gfs2/ops_fstype.c:   mapping->host = sb->s_bdev->bd_inode;

Hmm.. that's weird.  I cannot quite follow what is happening there.
It creates an address-space for metadata which doesn't have a real
inode, and borrows bits of the bdev inode ... possibly just to be able
to find the blocksize deep in buffer.c or similar.
I suspect that using an 'inode' instead of a 'mapping' would make the
code clearer.

> fs/nilfs2/page.c:       mapping->host = inode;

A nilfs inode is allocated with 2 address spaces,
one for the data and one for btree indexing metadata.
And then there are a couple of extra address spaces for the
global metadata-file (mtd).

I wonder what the ->host pointer is actually used for.
buffer.c uses it:
 - to mark the inode 'dirty' when the page is marked dirty
 - to find the blocksize of the inode, for creating buffer_heads
 - find the size of the mapping (i_size)

I could probably argue that the 'dirty' flag (at least for the data) and
the size really belong in the address_space, not in the inode.
The blocksize, I'm less sure of.

I suspect gfs2 and nilfs2 could be changed to allocate a separate inode
(instead of address_space), or to not make use of the ->host pointer.
It would be more work than I at first thought though.

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)