linux-ext4 - Re: [RFC][Patch 1/2] Persistent preallocation in ext4

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]

Date:	Tue, 2 Jan 2007 16:34:09 +0530
From:	"Amit K. Arora" <aarora@...ux.vnet.ibm.com>
To:	Mingming Cao <cmm@...ibm.com>
Cc:	linux-ext4@...r.kernel.org, suparna@...ibm.com, suzuki@...ibm.com,
	alex@...sterfs.com
Subject: Re: [RFC][Patch 1/2] Persistent preallocation in ext4

Hi Mingming,

On Wed, Dec 27, 2006 at 03:30:44PM -0800, Mingming Cao wrote:
> looks good to me, a few comments :)
Thanks for your comments!

> .....
> > +	  		ret = ext4_ext_get_blocks(handle, inode, block,
> > +					max_blocks, &map_bh,
> > +					EXT4_CREATE_UNINITIALIZED_EXT, 0);
> > +			if(ret > 0 && test_bit(BH_New, &map_bh.b_state))
> > +				nblocks = nblocks + ret;
> > +		}
> 
> 
> ext4_ext_get_blocks() returns 0 when it is mapping (non allocating) a
> hole. In our case, we are doing allocating, so here it is not possible
> to returns a 0 from ext4_ext_get_blocks(). I think we should quit the
> loop and BUGON if ret == 0 here.

Okay. I will add "BUG_ON(!ret);" just after get_blocks, above.

> 
> > +		if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb,
> > +						&retries))
> > +			goto retry;
> > +
> > +		if(nblocks) {
> > +			mutex_lock(&inode->i_mutex);
> > +			inode->i_size = inode->i_size + (nblocks >> blkbits);
> > +			EXT4_I(inode)->i_disksize = inode->i_size;
> > +			mutex_unlock(&inode->i_mutex);
> > +		}
> 
> Hmm... We should not need to worry about the inode->i_size if we are
> preallocating blocks for holes.

You are right. Will take care of this.
 
> And, Looking at other places calling ext4_*_get_blocks() in the kernel,
> it seems not all of them protected by i_mutex lock. I think it probably
> okay to not holding i_mutex during calling ext4_ext4_get_blocks().

We are not holding i_mutex lock during ext4_ext_get_blocks() call.
Above, this lock is being held inorder to avoid race while updating the
filesize in inode (reference your comment in a previous mail "I think we
should update i_size and i_disksize after preallocation. Oh,
to protect parallel updating i_size, we have to take i_mutex down.").
Perhaps, truncate_mutex lock should be used here, and not i_mutex.

> 
> > +
> > +		ext4_mark_inode_dirty(handle, inode);
> > +		ret2 = ext4_journal_stop(handle);
> > +		if(ret > 0)
> > +			ret = ret2;
> > +
> > +		return ret > 0 ? nblocks : ret;
> > +	}
> > +
> 
> Since the API takes the number of bytes to preallocate, at return time,
> shall we convert the blocks to bytes to the user?
> 
> Here it returns the number of allocated blocks to the user.   Do we need
> to worry about the case when dealing with a range with partial hole and
> partial blocks already allocated? In that case nblocks(the new
> preallocated blocks) will less than the maxblocks (the number of blocks
> asked by application).  I am wondering what does other filesystem like
> xfs do? Maybe we should do the same thing.

I think xfs just returns 0 on success, and errno on an error. Do we
want to keep the same behavior here ? Or, should we return the number of
bytes preallocated ?

Thanks!

--
Regards,
Amit Arora
-
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