Re: [PATCH] iomap: add a swapfile activation function

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, Apr 21, 2018 at 02:33:01PM +0200, Jan Kara wrote:
> On Tue 17-04-18 19:50:23, Darrick J. Wong wrote:
> > +
> > +/* Swapfile activation */
> > +
> > +struct iomap_swapfile_info {
> > +	struct swap_info_struct *sis;
> > +	uint64_t lowest_ppage;		/* lowest physical addr seen (pages) */
> > +	uint64_t highest_ppage;		/* highest physical addr seen (pages) */
> > +	unsigned long expected_page_no;	/* next logical offset wanted (pages) */
> > +	int nr_extents;			/* extent count */
> > +};
> > +
> > +static loff_t iomap_swapfile_activate_actor(struct inode *inode, loff_t pos,
> > +		loff_t count, void *data, struct iomap *iomap)
> > +{
> > +	struct iomap_swapfile_info *isi = data;
> > +	unsigned long page_no = iomap->offset >> PAGE_SHIFT;
> > +	unsigned long nr_pages = iomap->length >> PAGE_SHIFT;
> > +	uint64_t first_ppage = iomap->addr >> PAGE_SHIFT;
> > +	uint64_t last_ppage = ((iomap->addr + iomap->length) >> PAGE_SHIFT) - 1;
> > +
> > +	/* Only one bdev per swap file. */
> > +	if (iomap->bdev != isi->sis->bdev)
> > +		goto err;
> > +
> > +	/* Must be aligned to a page boundary. */
> > +	if ((iomap->offset & ~PAGE_MASK) || (iomap->addr & ~PAGE_MASK) ||
> > +	    (iomap->length & ~PAGE_MASK))
> > +		goto err;
> 
> Reporting error in this case does not look equivalent to
> generic_swapfile_activate()? That function just skips blocks with
> insufficient alignment...

Yes, I've fixed this to emulate more closely the behavior of the old
bmap implementation, so now we collect physically contiguous iomaps
and trim the accumulated iomap to satisfy the page alignment
requirements.

> And I'm actually puzzled why alignment of physical block is needed but
> that's independent question.

I'm not sure why either. :)

> > +	/* Only real or unwritten extents. */
> > +	if (iomap->type != IOMAP_MAPPED && iomap->type != IOMAP_UNWRITTEN)
> > +		goto err;
> > +
> > +	/* No sparse files. */
> > +	if (isi->expected_page_no != page_no)
> > +		goto err;
> > +
> > +	/* No uncommitted metadata or shared blocks or inline data. */
> > +	if (iomap->flags & (IOMAP_F_DIRTY | IOMAP_F_SHARED |
> > +			    IOMAP_F_DATA_INLINE))
> > +		goto err;
> > +
> > +	/*
> > +	 * Calculate how much swap space we're adding; the first page contains
> > +	 * the swap header and doesn't count.
> > +	 */
> > +	if (page_no == 0)
> > +		first_ppage++;
> > +	if (isi->lowest_ppage > first_ppage)
> > +		isi->lowest_ppage = first_ppage;
> > +	if (isi->highest_ppage < last_ppage)
> > +		isi->highest_ppage = last_ppage;
> > +
> > +	/* Add extent, set up for the next call. */
> > +	isi->nr_extents += add_swap_extent(isi->sis, page_no, nr_pages,
> > +			first_ppage);
> 
> And here add_swap_extent() can return error.

Fixed, thanks.

--D

> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR