linux-kernel - Re: [RFC PATCH V3] iomap: add support to track dirty state of sub pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <43dc04bf-17bb-9f15-4f1c-dfd6c47c3fb1@huawei.com>
Date:   Thu, 20 Aug 2020 09:38:53 +0800
From:   "yukuai (C)" <yukuai3@...wei.com>
To:     Gao Xiang <hsiangkao@...hat.com>
CC:     <hch@...radead.org>, <darrick.wong@...cle.com>,
        <willy@...radead.org>, <david@...morbit.com>,
        <linux-xfs@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>
Subject: Re: [RFC PATCH V3] iomap: add support to track dirty state of sub
 pages

On 2020/8/19 20:56, Gao Xiang wrote:
> On Wed, Aug 19, 2020 at 08:05:42PM +0800, Yu Kuai wrote:
> 
> ...
> 
>> +static void
>> +iomap_iop_set_range_dirty(struct page *page, unsigned int off,
>> +		unsigned int len)
>> +{
>> +	struct iomap_page *iop = to_iomap_page(page);
>> +	struct inode *inode = page->mapping->host;
>> +	unsigned int first = DIRTY_BITS(off >> inode->i_blkbits);
>> +	unsigned int last = DIRTY_BITS((off + len - 1) >> inode->i_blkbits);
>> +	unsigned long flags;
>> +	unsigned int i;
>> +
>> +	spin_lock_irqsave(&iop->state_lock, flags);
>> +	for (i = first; i <= last; i++)
>> +		set_bit(i, iop->state);
>> +
>> +	if (last >= first)
>> +		iomap_set_page_dirty(page);
> 
> set_page_dirty() in the atomic context?
> 

Hi,

You'are right, this shouldn't be inside spin_lock.

>> +
>> +	spin_unlock_irqrestore(&iop->state_lock, flags);
>> +}
>> +
>> +static void
>> +iomap_set_range_dirty(struct page *page, unsigned int off,
>> +		unsigned int len)
>> +{
>> +	if (PageError(page))
>> +		return;
>> +
>> +	if (page_has_private(page))
>> +		iomap_iop_set_range_dirty(page, off, len);
> 
> 
> I vaguely remembered iomap doesn't always set up PagePrivate.
>

If so, maybe I should move iomap_set_page_dirty() to
ioamp_set_range_dirty().

Thanks,
Yu Kuai

> 
> @@ -705,7 +770,7 @@ __iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
>   	if (unlikely(copied < len && !PageUptodate(page)))
>   		return 0;
>   	iomap_set_range_uptodate(page, offset_in_page(pos), len);
> -	iomap_set_page_dirty(page);
> +	iomap_set_range_dirty(page, offset_in_page(pos), len);
>   	return copied;
>   }
> 
> so here could be suspectable, but I might be wrong here since
> I just take a quick look.
> 
> Thanks,
> Gao Xiang
> 
> 
> .
>