On Mon, 2009-09-07 at 23:57 +0200, Jan Kara wrote: > Hi Minming, > > the patch looks cleaner now. > Hi Jan Thanks for your review:) > On Thu 03-09-09 17:44:50, Mingming wrote: > > Index: linux-2.6.31-rc4/fs/ext4/inode.c > > =================================================================== > > --- linux-2.6.31-rc4.orig/fs/ext4/inode.c > > +++ linux-2.6.31-rc4/fs/ext4/inode.c > ... > > +#define DIO_AIO 0x1 > This flag isn't set anywhere... > > > +static void ext4_free_io_end(ext4_io_end_t *io) > > +{ > > + kfree(io); > > +} > > + > > +/* > > + * IO write completion for unwritten extents. > > + * > > + * check a range of space and convert unwritten extents to written. > > + */ > > +static void ext4_end_dio_unwritten(struct work_struct *work) > > +{ > > + ext4_io_end_t *io = container_of(work, ext4_io_end_t, work); > > + struct inode *inode = io->inode; > > + loff_t offset = io->offset; > > + size_t size = io->size; > > + int ret = 0; > > + int aio = io->flag & DIO_AIO; > > + > > + if (aio) > > + mutex_lock(&inode->i_mutex); > > + if (offset + size <= i_size_read(inode)) > > + ret = ext4_convert_unwritten_extents(inode, offset, size); > > + > > + if (ret < 0) > > + printk(KERN_EMERG "%s: failed to convert unwritten" > > + "extents to written extents, error is %d\n", > > + __func__, ret); > > + > > + ext4_free_io_end(io); > > + if (aio) > > + mutex_unlock(&inode->i_mutex); > > +} > > + > > +static ext4_io_end_t *ext4_init_io_end (struct inode *inode, unsigned int flag) > > +{ > > + ext4_io_end_t *io = NULL; > > + > > + io = kmalloc(sizeof(*io), GFP_NOFS); > > + > > + if (io) { > > + io->inode = inode; > You should __iget() the inode here and iput() it in the end IO handler at > least in the AIO case so that the inode remains pinned in memory. > I just tried to add this, but hit some errors. I will dig more. > > + io->flag = flag; > > + io->offset = 0; > > + io->size = 0; > > + io->error = 0; > > + INIT_WORK(&io->work, ext4_end_dio_unwritten); > > + } > > + > > + return io; > > +} > > + > > +static void ext4_end_io_dio(struct kiocb *iocb, loff_t offset, > > + ssize_t size, void *private) > > +{ > > + ext4_io_end_t *io_end = iocb->private; > > + struct workqueue_struct *wq; > > + > > + /* if not hole or unwritten extents, just simple return */ > > + if (!io_end || !size || !iocb->private) > > + return; > > + io_end->offset = offset; > > + io_end->size = size; > > + wq = EXT4_SB(io_end->inode->i_sb)->dio_unwritten_wq; > > + > > + /* We need to convert unwritten extents to written */ > > + queue_work(wq, &io_end->work); > > + > > + if (is_sync_kiocb(iocb)) > > + flush_workqueue(wq); > You have spaces instead of tabs above. > > I think fsync() still won't work correctly since it can happen user sees > AIO completed, calls fsync() that completes, hmm, does fsync() ensure user sees AIO data completed? > but the conversion of extents > still has not happened because the conversion thread as not run yet. > The simple solution would be so flush_workqueue() from ext4_sync_fs() > and ext4_fsync(). But that would needlessly make fsync() wait for > conversion in unrelated files. More sophisticated solution would be to > attach io_end structures to the inode and do the work described by them > in ext4_fsync() (ext4_sync_fs() is fine doing flush_workqueue() since it > has to do all the work anyway). But in this solution you would have to be > careful to avoid races of fsync() and the completion thread. Yeah maybe link the list of io_end structures to the inode, and use a lock to protect that link... All AIO related issues (including ext3) seems should addressed in another patch, you agree? Or we should address them here? Mingming > Besides these problems the patch looks good. > > Honza -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html