linux-ext4 - Re: [PATCH] ext4: Fix discard of inode prealloc space with delayed allocation.

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: <20090225173740.GA10367@skywalker>
Date:	Wed, 25 Feb 2009 23:07:40 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Dmitri Monakhov <dmonakhov@...nvz.org>
Cc:	tytso@....edu, linux-ext4@...r.kernel.org
Subject: Re: [PATCH] ext4: Fix discard of inode prealloc space with delayed
	allocation.

On Wed, Feb 25, 2009 at 07:57:52PM +0300, Dmitri Monakhov wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com> writes:
> 
> > With delayed allocation we should not/cannot discard inode prealloc space
> > during file close. We would still have dirty pages for which we haven't allocated
> > blocks yet. With this fix after each get_blocks request we check whether we have
> > zero reserved blocks and if yes and we don't have any writers on the file we
> > discard inode prealloc space.
> >
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
> >
> > ---
> >  fs/ext4/file.c  |    9 ++++++++-
> >  fs/ext4/inode.c |    6 ++++++
> >  2 files changed, 14 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index f731cb5..4e468e2 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -33,9 +33,16 @@
> >   */
> >  static int ext4_release_file(struct inode *inode, struct file *filp)
> >  {
> > +	int rsv_data_blocks;
> > +
> > +	spin_lock(&EXT4_I(inode)->i_block_reservation_lock);
> > +	rsv_data_blocks = EXT4_I(inode)->i_reserved_data_blocks;
> > +	spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> > +
> Seems we have race condition here because at this point someone may:
> 1)open file
> 2)then perform some write activity => (i_reserved_data_blocks != 0)
> 3)close file => (inode->i_writecount == 1)
> 

i_data_sem doesn't actually ensure i_reserved_data_blocks won't change.
We update i_reserved_data_blocks during block reservation
(ext4_da_write_begin). But yes what you mentioned is possible.
But I am not sure we need to be worried about that. It is true that
throwing away prealloc space is not good. But we still do block allocation
based on goal blocks. So we may end up allocating blocks within the
discarded prealloc space again.

What is bad is that if we don't discard the prealloc space even after
all the needed block allocation. That would mean nothing can be
allocated out of that prealloc space. With the current code We would
be discarding the prealloc space only when we hit ENOSPC 
. But then that would be bad.

-aneesh

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html