On Thu, Dec 31, 2020 at 06:23:58PM +0300, Konstantin Komarov wrote:
> This adds NTFS journal
>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> ---
> fs/ntfs3/fslog.c | 5220 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 5220 insertions(+)
> create mode 100644 fs/ntfs3/fslog.c
>
> diff --git a/fs/ntfs3/fslog.c b/fs/ntfs3/fslog.c
> +static int read_log_page(struct ntfs_log *log, u32 vbo,
> + struct RECORD_PAGE_HDR **buffer, bool allow_errors,
> + bool ignore_usa_error, bool *usa_error)
Allow_errors does nothing. I also think that no need for
ignore_usa_error. We can just check usa_error if we need
it. We just never raise return error for usa_error. And
then caller can decide if want's to use it.
> +{
> + int err = 0;
> + u32 page_idx = vbo >> log->page_bits;
> + u32 page_off = vbo & log->page_mask;
> + u32 bytes = log->page_size - page_off;
> + void *to_free = NULL;
> + u32 page_vbo = page_idx << log->page_bits;
> + struct RECORD_PAGE_HDR *page_buf;
> + struct ntfs_inode *ni = log->ni;
> + bool bBAAD;
> +
> + if (vbo >= log->l_size)
> + return -EINVAL;
> +
> + if (!*buffer) {
> + to_free = ntfs_alloc(bytes, 0);
> + if (!to_free)
> + return -ENOMEM;
> + *buffer = to_free;
> + }
> +
> + page_buf = page_off ? log->one_page_buf : *buffer;
> +
> + err = ntfs_read_run_nb(ni->mi.sbi, &ni->file.run, page_vbo, page_buf,
> + log->page_size, NULL);
> + if (err)
> + goto out;
> +
> + if (page_buf->rhdr.sign != NTFS_FFFF_SIGNATURE)
> + ntfs_fix_post_read(&page_buf->rhdr, PAGE_SIZE, false);
> +
> + if (page_buf != *buffer)
> + memcpy(*buffer, Add2Ptr(page_buf, page_off), bytes);
> +
> + bBAAD = page_buf->rhdr.sign == NTFS_BAAD_SIGNATURE;
> +
> + /* Check that the update sequence array for this page is valid */
> + if (bBAAD) {
> + /* If we don't allow errors, raise an error status */
> + if (!ignore_usa_error) {
> + err = -EINVAL;
> + goto out;
> + }
> + }
> +
> + if (usa_error)
> + *usa_error = bBAAD;
> +
So here we can just
delete if(bBAAD)
and use
if (usa_error)
*usa_error = page_buf->rhdr.sign == NTFS_BAAD_SIGNATURE;
> +out:
> + if (err && to_free) {
> + ntfs_free(to_free);
> + *buffer = NULL;
> + }
> +
> + return err;
> +}
> +/*
> + * last_log_lsn
> + *
> + * This routine walks through the log pages for a file, searching for the
> + * last log page written to the file
> + */
> +static int last_log_lsn(struct ntfs_log *log)
> +{
> + struct RECORD_PAGE_HDR *first_tail = NULL;
> + struct RECORD_PAGE_HDR *second_tail = NULL;
> +next_tail:
> + /* Read second tail page (at pos 3/0x12000) */
> + if (read_log_page(log, second_off, &second_tail, true, true,
> + &usa_error) ||
> + usa_error || second_tail->rhdr.sign != NTFS_RCRD_SIGNATURE) {
> + ntfs_free(second_tail);
> + second_tail = NULL;
> + second_file_off = 0;
> + lsn2 = 0;
> + } else {
> + second_file_off = hdr_file_off(log, second_tail);
> + lsn2 = le64_to_cpu(second_tail->record_hdr.last_end_lsn);
> + }
What will happend if we get -ENOMEM from read_log_page(). Log page
might still be valid we will just ignore it. This doesn't sound
right.
This same thing happens many place with read_log_page().
> +
> + /* Read first tail page (at pos 2/0x2000 ) */
> + if (read_log_page(log, final_off, &first_tail, true, true,
> + &usa_error) ||
> + usa_error || first_tail->rhdr.sign != NTFS_RCRD_SIGNATURE) {
> + ntfs_free(first_tail);
> + first_tail = NULL;
> + first_file_off = 0;
> + lsn1 = 0;
> + } else {
> + first_file_off = hdr_file_off(log, first_tail);
> + lsn1 = le64_to_cpu(first_tail->record_hdr.last_end_lsn);
> + }
> + if (first_tail && second_tail) {
> + if (best_lsn1 > best_lsn2) {
> + best_lsn = best_lsn1;
> + best_page = first_tail;
> + this_off = first_file_off;
> + } else {
> + best_lsn = best_lsn2;
> + best_page = second_tail;
> + this_off = second_file_off;
> + }
> + } else if (first_tail) {
> + best_lsn = best_lsn1;
> + best_page = first_tail;
> + this_off = first_file_off;
> + } else if (second_tail) {
> + best_lsn = best_lsn2;
> + best_page = second_tail;
> + this_off = second_file_off;
> + } else {
> + goto free_and_tail_read;
Can't we just use straight tail_read here?
> + }
> +
> + best_page_pos = le16_to_cpu(best_page->page_pos);
> + } else {
> +free_and_tail_read:
> + ntfs_free(first_tail);
> + ntfs_free(second_tail);
> + goto tail_read;
> + }
> +
> + ntfs_free(first_tail_prev);
> + first_tail_prev = first_tail;
> + final_off_prev = first_file_off;
> + first_tail = NULL;
> +
> + ntfs_free(second_tail_prev);
> + second_tail_prev = second_tail;
> + second_off_prev = second_file_off;
> + second_tail = NULL;
> +
> + final_off += log->page_size;
> + second_off += log->page_size;
> +
> + if (tails < 0x10)
> + goto next_tail;
> +tail_read:
> + first_tail = first_tail_prev;
> + final_off = final_off_prev;
> +int log_replay(struct ntfs_inode *ni)
> +{
> + /* Now we need to walk through looking for the last lsn */
> + err = last_log_lsn(log);
> + if (err == -EROFS)
> + goto out;
> +
No need for this if below is whole err check.
> + if (err)
> + goto out;
