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
| ||
|
Message-ID: <alpine.LFD.2.00.1405291439340.18491@localhost.localdomain> Date: Thu, 29 May 2014 14:42:04 +0200 (CEST) From: Lukáš Czerner <lczerner@...hat.com> To: Namjae Jeon <namjae.jeon@...sung.com> cc: "Theodore Ts'o" <tytso@....edu>, linux-ext4 <linux-ext4@...r.kernel.org>, Ashish Sangwan <a.sangwan@...sung.com> Subject: Re: [PATCH 1/2] ext4: introduce new i_write_mutex to protect fallocate On Tue, 13 May 2014, Namjae Jeon wrote: > Date: Tue, 13 May 2014 09:19:17 +0900 > From: Namjae Jeon <namjae.jeon@...sung.com> > To: Theodore Ts'o <tytso@....edu> > Cc: linux-ext4 <linux-ext4@...r.kernel.org>, > Lukáš Czerner <lczerner@...hat.com>, > Ashish Sangwan <a.sangwan@...sung.com> > Subject: [PATCH 1/2] ext4: introduce new i_write_mutex to protect fallocate > > Introduce new i_write_mutex to protect new writes from coming while doing > fallocate operations. Also, get rid of aio_mutex as it is covered by > i_write_mutex. I wonder what is the performance impact of this change ? Especially since we're not longer taking the lock only in unaligned aio/dio case but in all cases ? Also, against what tree is this patch ? The description is quite sparse and I would like see a reasoning for this change, because it's completely missing! Thanks! -Lukas > > Signed-off-by: Namjae Jeon <namjae.jeon@...sung.com> > Signed-off-by: Ashish Sangwan <a.sangwan@...sung.com> > --- > fs/ext4/ext4.h | 6 +++--- > fs/ext4/extents.c | 18 +++++++++++++++--- > fs/ext4/file.c | 18 +++++++++++------- > fs/ext4/inode.c | 7 ++++++- > fs/ext4/super.c | 3 +-- > 5 files changed, 36 insertions(+), 16 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 6b45afa..77e5705 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -943,6 +943,9 @@ struct ext4_inode_info { > > /* Precomputed uuid+inum+igen checksum for seeding inode checksums */ > __u32 i_csum_seed; > + > + /* protects fallocate operations racing with new writes */ > + struct mutex i_write_mutex; > }; > > /* > @@ -2827,10 +2830,7 @@ static inline void ext4_inode_resume_unlocked_dio(struct inode *inode) > #define EXT4_WQ_HASH_SZ 37 > #define ext4_ioend_wq(v) (&ext4__ioend_wq[((unsigned long)(v)) %\ > EXT4_WQ_HASH_SZ]) > -#define ext4_aio_mutex(v) (&ext4__aio_mutex[((unsigned long)(v)) %\ > - EXT4_WQ_HASH_SZ]) > extern wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ]; > -extern struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ]; > > #define EXT4_RESIZING 0 > extern int ext4_resize_begin(struct super_block *sb); > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c > index 086baa9..5262750 100644 > --- a/fs/ext4/extents.c > +++ b/fs/ext4/extents.c > @@ -4741,6 +4741,8 @@ static long ext4_zero_range(struct file *file, loff_t offset, > if (!S_ISREG(inode->i_mode)) > return -EINVAL; > > + mutex_lock(&EXT4_I(inode)->i_write_mutex); > + > /* > * Write out all dirty pages to avoid race conditions > * Then release them. > @@ -4748,8 +4750,10 @@ static long ext4_zero_range(struct file *file, loff_t offset, > if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > ret = filemap_write_and_wait_range(mapping, offset, > offset + len - 1); > - if (ret) > + if (ret) { > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > + } > } > > /* > @@ -4761,8 +4765,10 @@ static long ext4_zero_range(struct file *file, loff_t offset, > start = round_up(offset, 1 << blkbits); > end = round_down((offset + len), 1 << blkbits); > > - if (start < offset || end > offset + len) > + if (start < offset || end > offset + len) { > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return -EINVAL; > + } > partial = (offset + len) & ((1 << blkbits) - 1); > > lblk = start >> blkbits; > @@ -4859,6 +4865,7 @@ out_dio: > ext4_inode_resume_unlocked_dio(inode); > out_mutex: > mutex_unlock(&inode->i_mutex); > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > } > > @@ -5428,11 +5435,15 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len) > punch_start = offset >> EXT4_BLOCK_SIZE_BITS(sb); > punch_stop = (offset + len) >> EXT4_BLOCK_SIZE_BITS(sb); > > + mutex_lock(&EXT4_I(inode)->i_write_mutex); > + > /* Call ext4_force_commit to flush all data in case of data=journal. */ > if (ext4_should_journal_data(inode)) { > ret = ext4_force_commit(inode->i_sb); > - if (ret) > + if (ret) { > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > + } > } > > /* > @@ -5518,5 +5529,6 @@ out_dio: > ext4_inode_resume_unlocked_dio(inode); > out_mutex: > mutex_unlock(&inode->i_mutex); > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > } > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index 708aad7..557b4ac 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -93,7 +93,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > { > struct file *file = iocb->ki_filp; > struct inode *inode = file_inode(iocb->ki_filp); > - struct mutex *aio_mutex = NULL; > + bool unaligned_direct_aio = false; > struct blk_plug plug; > int o_direct = file->f_flags & O_DIRECT; > int overwrite = 0; > @@ -101,6 +101,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > ssize_t ret; > loff_t pos = iocb->ki_pos; > > + mutex_lock(&EXT4_I(inode)->i_write_mutex); > + > /* > * Unaligned direct AIO must be serialized; see comment above > * In the case of O_APPEND, assume that we must always serialize > @@ -110,8 +112,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > !is_sync_kiocb(iocb) && > (file->f_flags & O_APPEND || > ext4_unaligned_aio(inode, from, pos))) { > - aio_mutex = ext4_aio_mutex(inode); > - mutex_lock(aio_mutex); > + unaligned_direct_aio = true; > ext4_unwritten_wait(inode); > } > > @@ -143,8 +144,9 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > iocb->private = &overwrite; > > /* check whether we do a DIO overwrite or not */ > - if (ext4_should_dioread_nolock(inode) && !aio_mutex && > - !file->f_mapping->nrpages && pos + length <= i_size_read(inode)) { > + if (ext4_should_dioread_nolock(inode) && > + !unaligned_direct_aio && !file->f_mapping->nrpages && > + pos + length <= i_size_read(inode)) { > struct ext4_map_blocks map; > unsigned int blkbits = inode->i_blkbits; > int err, len; > @@ -174,6 +176,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > > ret = __generic_file_write_iter(iocb, from); > mutex_unlock(&inode->i_mutex); > + if (!unaligned_direct_aio) > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > > if (ret > 0) { > ssize_t err; > @@ -186,8 +190,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > blk_finish_plug(&plug); > > errout: > - if (aio_mutex) > - mutex_unlock(aio_mutex); > + if (unaligned_direct_aio) > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > } > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index b1dc334..d804120 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3528,6 +3528,8 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length) > > trace_ext4_punch_hole(inode, offset, length, 0); > > + mutex_lock(&EXT4_I(inode)->i_write_mutex); > + > /* > * Write out all dirty pages to avoid race conditions > * Then release them. > @@ -3535,8 +3537,10 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length) > if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) { > ret = filemap_write_and_wait_range(mapping, offset, > offset + length - 1); > - if (ret) > + if (ret) { > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > + } > } > > mutex_lock(&inode->i_mutex); > @@ -3637,6 +3641,7 @@ out_dio: > ext4_inode_resume_unlocked_dio(inode); > out_mutex: > mutex_unlock(&inode->i_mutex); > + mutex_unlock(&EXT4_I(inode)->i_write_mutex); > return ret; > } > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 1f8cb18..e236c85 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -904,6 +904,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb) > atomic_set(&ei->i_ioend_count, 0); > atomic_set(&ei->i_unwritten, 0); > INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work); > + mutex_init(&ei->i_write_mutex); > > return &ei->vfs_inode; > } > @@ -5505,7 +5506,6 @@ static void ext4_exit_feat_adverts(void) > > /* Shared across all ext4 file systems */ > wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ]; > -struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ]; > > static int __init ext4_init_fs(void) > { > @@ -5518,7 +5518,6 @@ static int __init ext4_init_fs(void) > ext4_check_flag_values(); > > for (i = 0; i < EXT4_WQ_HASH_SZ; i++) { > - mutex_init(&ext4__aio_mutex[i]); > init_waitqueue_head(&ext4__ioend_wq[i]); > } > >
Powered by blists - more mailing lists