On Tue 13-06-23 09:27:38, Ritesh Harjani wrote: > Matthew Wilcox <willy@xxxxxxxxxxxxx> writes: > > On Mon, Jun 12, 2023 at 11:55:55PM +0530, Ritesh Harjani wrote: > >> Matthew Wilcox <willy@xxxxxxxxxxxxx> writes: > >> I couldn't respond to your change because I still had some confusion > >> around this suggestion - > >> > >> > So do we care if we write a random fragment of a page after a truncate? > >> > If so, we should add: > >> > > >> > if (folio_pos(folio) >= size) > >> > return 0; /* Do we need to account nr_to_write? */ > >> > >> I was not sure whether if go with above case then whether it will > >> work with collapse_range. I initially thought that collapse_range will > >> truncate the pages between start and end of the file and then > >> it can also reduce the inode->i_size. That means writeback can find an > >> inode->i_size smaller than folio_pos(folio) which it is writing to. > >> But in this case we can't skip the write in writeback case like above > >> because that write is still required (a spurious write) even though > >> i_size is reduced as it's corresponding FS blocks are not truncated. > >> > >> But just now looking at ext4_collapse_range() code it doesn't look like > >> it is the problem because it waits for any dirty data to be written > >> before truncate. So no matter which folio_pos(folio) the writeback is > >> writing, there should not be an issue if we simply return 0 like how > >> you suggested above. > >> > >> static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len) > >> > >> <...> > >> ioffset = round_down(offset, PAGE_SIZE); > >> /* > >> * Write tail of the last page before removed range since it will get > >> * removed from the page cache below. > >> */ > >> > >> ret = filemap_write_and_wait_range(mapping, ioffset, offset); > >> if (ret) > >> goto out_mmap; > >> /* > >> * Write data that will be shifted to preserve them when discarding > >> * page cache below. We are also protected from pages becoming dirty > >> * by i_rwsem and invalidate_lock. > >> */ > >> ret = filemap_write_and_wait_range(mapping, offset + len, > >> LLONG_MAX); > >> truncate_pagecache(inode, ioffset); > >> > >> <... within i_data_sem> > >> i_size_write(inode, new_size); > >> > >> <...> > >> > >> > >> However to avoid problems like this I felt, I will do some more code > >> reading. And then I was mostly considering your second suggestion which > >> is this. This will ensure we keep the current behavior as is and not > >> change that. > >> > >> > If we simply don't care that we're doing a spurious write, then we can > >> > do something like: > >> > > >> > - len = size & ~PAGE_MASK; > >> > + len = size & (len - 1); > > > > For all I know, I've found a bug here. I don't know enough about ext4; if > > we have truncated a file, and then writeback a page that is past i_size, > > will the block its writing to have been freed? > > I don't think so. If we look at truncate code, it first reduces i_size, > then call truncate_pagecache(inode, newsize) and then we will call > ext4_truncate() which will free the corresponding blocks. > Since writeback happens with folio lock held until completion, hence I > think truncate_pagecache() should block on that folio until it's lock > has been released. > > - IIUC, if truncate would have completed then the folio won't be in the > foliocache for writeback to happen. Foliocache is kept consistent > via > - first truncate the folio in the foliocache and then remove/free > the blocks on device. Yes, correct. > - Also the reason we update i_size "before" calling truncate_pagecache() > is to synchronize with mmap/pagefault. Yes, but these days mapping->invalidate_lock works for that instead for ext4. > > Is this potentially a silent data corruptor? > > - Let's consider a case when folio_pos > i_size but both still belongs > to the last block. i.e. it's a straddle write case. > In such case we require writeback to write the data of this last folio > straddling i_size. Because truncate will not remove/free this last folio > straddling i_size & neither the last block will be freed. And I think > writeback is supposed to write this last folio to the disk to keep the > cache and disk data consistent. Because truncate will only zero out > the rest of the folio in the foliocache. But I don't think it will go and > write that folio out (It's not required because i_size means that the > rest of the folio beyond i_size should remain zero). > > So, IMO writeback is supposed to write this last folio to the disk. And, > if we skip this writeout, then I think it may cause silent data corruption. > > But I am not sure about the rest of the write beyond the last block of > i_size. I think those could just be spurious writes which won't cause > any harm because truncate will eventually first remove this folio from > file mapping and then will release the corresponding disk blocks. > So writing those out should does no harm Correct. The block straddling i_size must be written out, the blocks fully beyond new i_size (but below old i_size) may or may not be written out. As you say these extra racing writes to blocks that will get truncated cause no harm. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
