On Wed, Apr 09, 2014 at 11:46:44AM +0200, Jan Kara wrote:
> Another day, some more review ;) Comments below.
I'm really grateful for all this review! It's killing me, though ;-)
> > +int dax_clear_blocks(struct inode *inode, sector_t block, long size)
> > +{
> > + struct block_device *bdev = inode->i_sb->s_bdev;
> > + const struct block_device_operations *ops = bdev->bd_disk->fops;
> > + sector_t sector = block << (inode->i_blkbits - 9);
> > + unsigned long pfn;
> > +
> > + might_sleep();
> > + do {
> > + void *addr;
> > + long count = ops->direct_access(bdev, sector, &addr, &pfn,
> > + size);
> So do you assume blocksize == PAGE_SIZE here? If not, addr could be in
> the middle of the page AFAICT.
You're right. Depending on how clear_page() is implemented, that
might go badly wrong. Of course, both ext2 & ext4 require block_size
== PAGE_SIZE right now, so anything else is by definition untested.
I've been trying to keep DAX free from that assumption, but obviously
haven't caught all the places.
How does this look?
typedef long (*direct_access_t)(struct block_device *, sector_t, void **,
unsigned long *pfn, long size);
int dax_clear_blocks(struct inode *inode, sector_t block, long size)
{
struct block_device *bdev = inode->i_sb->s_bdev;
direct_access_t direct_access = bdev->bd_disk->fops->direct_access;
sector_t sector = block << (inode->i_blkbits - 9);
unsigned long pfn;
might_sleep();
do {
void *addr;
long count = direct_access(bdev, sector, &addr, &pfn, size);
if (count < 0)
return count;
while (count > 0) {
unsigned pgsz = PAGE_SIZE - offset_in_page(addr);
if (pgsz > count)
pgsz = count;
if (pgsz < PAGE_SIZE)
memset(addr, 0, pgsz);
else
clear_page(addr);
addr += pgsz;
size -= pgsz;
count -= pgsz;
sector += pgsz / 512;
cond_resched();
}
} while (size);
return 0;
}
EXPORT_SYMBOL_GPL(dax_clear_blocks);
> > if (IS_DAX(inode)) {
> > /*
> > - * we need to clear the block
> > + * block must be initialised before we put it in the tree
> > + * so that it's not found by another thread before it's
> > + * initialised
> > */
> > - err = ext2_clear_xip_target (inode,
> > - le32_to_cpu(chain[depth-1].key));
> > + err = dax_clear_blocks(inode, le32_to_cpu(chain[depth-1].key),
> > + count << inode->i_blkbits);
> Umm 'count' looks wrong here. You want to clear only one block, don't
> you?
I think I got confused between ext2 and ext4 here. I do want to clear
only one block.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>