lkml.org 
[lkml]   [2014]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 06/22] Replace XIP read and write with DAX I/O
    On Wed, Apr 09, 2014 at 11:14:50AM +0200, Jan Kara wrote:
    > On Tue 08-04-14 16:21:02, Matthew Wilcox wrote:
    > > On Tue, Apr 08, 2014 at 07:56:00PM +0200, Jan Kara wrote:
    > > > > +static void dax_new_buf(void *addr, unsigned size, unsigned first,
    > > > > + loff_t offset, loff_t end, int rw)
    > > > > +{
    > > > > + loff_t final = end - offset + first; /* The final byte of the buffer */
    > > > > + if (rw != WRITE) {
    > > > > + memset(addr, 0, size);
    > > > > + return;
    > > > > + }
    > > > It seems counterintuitive to zero out "on-disk" blocks (it seems you'd do
    > > > this for unwritten blocks) when reading from them. Presumably it could also
    > > > have undesired effects on endurance of persistent memory. Instead I'd expect
    > > > that you simply zero out user provided buffer the same way as you do it for
    > > > holes.
    > >
    > > I think we have to zero it here, because the second time we call
    > > get_block() for a given block, it won't be BH_New any more, so we won't
    > > know that it's supposed to be zeroed.
    > But how can you have BH_New buffer when you didn't ask get_blocks() to
    > create any block? That would be a bug in the get_blocks() implementation...
    > Or am I missing something?

    Oh ... right. So just to be clear, we're looking at the case where
    we're doing a read of a filesystem block which is BH_Unwritten, but
    isn't a hole ... so it's been allocated on storage and not yet written.
    That's already treated as a hole:

    if (rw == WRITE) {
    ...
    } else {
    hole = !buffer_written(bh);
    }

    and dax_new_buf is only called in the !hole case.

    > OK, but there are filesystems which do the same thing as ext4 (e.g.
    > btrfs) and historically noone really cared. E.g. direct IO code advances
    > only by a single block regardless of what filesystem returns when the
    > buffer is unmapped. As you correctly mention, get_blocks() API isn't really
    > documented so noone has really defined what should happen when you ask
    > filesystem to map some blocks and there's a hole. I agree what XFS does
    > looks sensible and ext4 can do the same. Hopefully this gets cleaned up
    > when Dave finishes his new block mapping interface.

    I hope so too! The get_block() API has been the bane of my existance
    since Christmas :-)

    > This wouldn't quite work because even ext4_map_blocks() doesn't bother to
    > fill in 'map' when it finds a hole. But it won't be complicated to
    > propagate the information.

    Good point.

    > > It'll be kind of tricky to move it because 'len' is not necessarily
    > > a multiple of i_blkbits, so we can't necessarily maintain b_blocknr
    > > accurately.
    > Yeah, after I understood the code I also understood why you do it the way
    > you did. But we could do something like:
    > ...
    > + if (!len)
    > + break;
    > +
    > blocks = ((offset + len) >> inode->i_blkbits) -
    > (offset >> inode->i_blkbits);
    > bh->b_blocknr += blocks;
    > bh->b_size -= blocks << inode->i_blkbits;
    > + offset += len;
    > + copied += len;
    > + addr += len;
    > ...

    We could ... I'm not sure it's simpler though.

    > BTW: it might be good to store inode->i_blkbits in a local variable. It
    > makes some expressions shorter.

    Yes, good idea. Done.

    > BTW2: although direct IO uses 'offset' for position in file, the rest of
    > VFS uses 'pos' for that and that seems to be less overloaded term so for me
    > it would be easier if you used 'pos' instead of 'offset'. Just a
    > suggestion.

    Sure. Done.

    > > > > + if (rw == WRITE) {
    > > > > + if (!buffer_mapped(bh)) {
    > > > > + retval = -EIO;
    > > > > + break;
    > > > -EIO looks like a wrong error here. Or maybe it is the right one and it
    > > > only needs some explanation? The thing is that for direct IO some
    > > > filesystems choose not to fill holes for direct IO and fall back to
    > > > buffered IO instead (to avoid exposure of uninitialized blocks if the
    > > > system crashes after blocks have been added to a file but before they were
    > > > written out). For DAX you are pretty much free to define what you ask from
    > > > the get_blocks() (and this fallback behavior is somewhat disputed behavior
    > > > in direct IO case so you might want to differ here) but you should document
    > > > it somewhere.
    > >
    > > Hmm ... I thought that calling get_block() with the create argument would
    > > force the return of a bh with the Mapped bit set. Did I misunderstand that
    > > aspect of the undocumented get_block() API too?
    > As you mention the API is undocumented and not really designed. So
    > filesystems do whatever causes the generic code to do what they want (it's
    > a mess I know). In this case, I'm warning you there are filesystems which
    > refuse to fill in holes from the get_blocks() function passed to
    > blockdev_direct_IO() (even ext4 does this for inodes with old
    > indirect-block based on disk format). You can just define DAX fails
    > horribly in these case and I'm fine with that at least in this stage. If
    > someone bothers later, fallback to buffered IO can be implemented. But we
    > should document this somewhere.

    Urgh. Yeah, we should probably fall back to buffered I/O for that case.
    I'll stick a comment in dax.c for now, and we can fix it later.

    > > > > + if ((flags & DIO_LOCKING) && (rw == READ)) {
    > > > > + struct address_space *mapping = inode->i_mapping;
    > > > > + mutex_lock(&inode->i_mutex);
    > > > > + retval = filemap_write_and_wait_range(mapping, offset, end - 1);
    > > > > + if (retval) {
    > > > > + mutex_unlock(&inode->i_mutex);
    > > > > + goto out;
    > > > > + }
    > > > Is there a reason for this? I'd assume DAX has no pages in pagecache...
    > >
    > > There will be pages in the page cache for holes that we page faulted on.
    > > They must go! :-)
    > Well, but this will only writeback dirty pages and if I read the code
    > correctly those pages will never be dirty since dax_mkwrite() will replace
    > them. Or am I missing something?

    In addition to writing back dirty pages, filemap_write_and_wait_range()
    will evict clean pages. Unintuitive, I know, but it matches what the
    direct I/O path does. Plus, if we fall back to buffered I/O for holes
    (see above), then this will do the right thing at that time.



    \
     
     \ /
      Last update: 2014-04-10 04:01    [W:3.221 / U:1.648 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site