On Thu, Oct 16, 2014 at 02:56:25PM +0200, Mathieu Desnoyers wrote:
> > +#define EXT4_MOUNT_DAX 0x00200 /* Execute in place */
>
> Execute in place -> Direct Access stuff... (comment above)
Thanks! Fixed.
> > +static int ext4_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > +{
> > + return dax_fault(vma, vmf, ext4_get_block);
> > + /* Is this the right get_block? */
>
> perhaps this needs a TODO or FIXME or XXX to make sure an ext4
> maintainer does not miss this question.
Maybe I can ambush Ted in the halls tomorrow and find out? :-)
> > + .fsync = ext4_sync_file,
> > + .fallocate = ext4_fallocate,
>
> Perhaps adding comments saying that .splice_read and .splice_write are
> unavailable here would help understanding why we need a different file
> operations structure.
Good idea. Done.
> > +static void ext4_end_io_unwritten(struct buffer_head *bh, int uptodate)
> > +{
> > + struct inode *inode = bh->b_assoc_map->host;
> > + /* XXX: breaks on 32-bit > 16GB. Is that even supported? */
>
> Good question! It would be interesting to get an answer :)
Another thing to check tomorrow ...
> > + if (!uptodate)
> > + return;
> > + WARN_ON(!buffer_unwritten(bh));
> > + err = ext4_convert_unwritten_extents(NULL, inode, offset, bh->b_size);
>
> err is simply unused here, that does not look good (silent failure).
I don't think I can do more than WARN_ON here. Maybe we can change
b_end_io() to return an int instead of void ... I think Dave Chinner has
grand plans for changes in this area as part of replacing the buffer_head
abstraction.
> > @@ -3238,14 +3249,6 @@ static int ext4_block_zero_page_range(handle_t *handle,
> > return -ENOMEM;
> >
> > blocksize = inode->i_sb->s_blocksize;
> > - max = blocksize - (offset & (blocksize - 1));
> > -
> > - /*
> > - * correct length if it does not fall between
> > - * 'from' and the end of the block
> > - */
> > - if (length > max || length < 0)
> > - length = max;
> >
> > iblock = index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits);
[...]
> > +
> > + /*
> > + * correct length if it does not fall between
> > + * 'from' and the end of the block
> > + */
>
> Shouldn't a length < 0 be treated as an error instead ?
>
> > + if (length > max || length < 0)
> > + length = max;
Monkey see code in wrong place. Monkey move code. monkey not understand
code.
> > @@ -3572,6 +3579,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > "both data=journal and dioread_nolock");
> > goto failed_mount;
> > }
> > + if (test_opt(sb, DAX)) {
> > + ext4_msg(sb, KERN_ERR, "can't mount with "
> > + "both data=journal and dax");
>
> This limitation regarding ext4 and dax should be documented in dax
> Documentation.
Maybe the ext4 documentation too? It seems kind of obvious to me that if
ypu're enabling in-place-updates that you can't journal the data you're
updating (well ... you could implement undo-log journalling, I suppose,
which would be quite a change for ext4)
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
